Skip to content

Make SlotImpl detachable from its owner, and add a new runOnAllThreads interface to Slot.#8135

Merged
alyssawilk merged 15 commits intoenvoyproxy:masterfrom
stevenzzzz:detachable-slot
Sep 11, 2019
Merged

Make SlotImpl detachable from its owner, and add a new runOnAllThreads interface to Slot.#8135
alyssawilk merged 15 commits intoenvoyproxy:masterfrom
stevenzzzz:detachable-slot

Conversation

@stevenzzzz
Copy link
Copy Markdown
Contributor

@stevenzzzz stevenzzzz commented Sep 3, 2019

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants