Make SlotImpl detachable from its owner, and add a new runOnAllThreads interface to Slot.#8135
Merged
alyssawilk merged 15 commits intoenvoyproxy:masterfrom Sep 11, 2019
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
See the issue in #7902, this PR is to make the SlotImpl detachable from its owner, by introducing a Booker object wraps around a SlotImpl, which bookkeeps all the on-the-fly update callbacks. And on its destruction, if there are still on-the-fly callbacks, move the SlotImpl to an deferred-delete queue, instead of destructing the SlotImpl which may cause an SEGV error.
More importantly, introduce a new runOnAllThreads(ThreadLocal::UpdateCb cb) API to Slot, which requests a Slot Owner to not assume that the Slot or its owner will out-live (in Main thread) the fired on-the-fly update callbacks, and should not capture the Slot or its owner in the update_cb.
Introducing an new runOnAllThreads(UpdateCb) api to Slot interface, for which a update callback is
expected to receive the currently stored ThreadLocal object, the callback may update or return a new ThreadLocal object that will be stored in the slot.
A Bookkeeper wraps around SlotImpl, which bookkeeps how many outgoing callbacks are on-the-fly. On its destruction been called in main thread, if the number doesn't go to zero, it will put the wrapped SlotImpl and its use count into a deffered delete queue.
In TLS InstanceImpl, add a deferred delete queue, a scheduleCleanup method will post a callback to main dispatcher. The callback deletes SlotImpl from the deferred delete queue, and reschedule another cleanup if the queue is not empty.
Picked RDS and config-providers-framework as examples to demonstrate that this change works. {i.e., changed from the runOnAllThreads(Event::PostCb) to the new runOnAllThreads(TLS::UpdateCb) interface. }
If the PR looks good, I will migrate existing (probably not all?) call-sites of runOnAllThreads to the new form in a follow up PR.
Risk Level: Medium
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #7902