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

[stevenzzzz]

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

[stevenzzzz]

/assign @mattklein123

[stevenzzzz]

@htuch FYI. :)

[mattklein123]

Thanks, this is an interesting solution. I've flushed out some comments assuming we go with this solution.

Stepping back though, can you refresh my memory on why this complexity is needed vs. just cancelling any potential updates before we start listener shutdown which I think would be serialized correctly? Thank you!

/wait

[mattklein123]

nit: const ThreadLocal::ThreadLocalObjectSharedPtr&

[stevenzzzz]

I want to express the idea that: ThreadLocal::Slot::UpdateCb is expected to receive data stored in the slot, and do-whatever-necessary, then return the new/updated version data to be saved in the slot. A non-const ThreadLocalObjectSharedPtr seems better under this notion, WDYT?

[mattklein123]

By passing by value you are essentially just forcing a copy in most cases. Personally, I think it's more common to make a new thing than use the previous, so I would pass by const-ref, and then if a copy needs to be made it can be done inside the function to put the cost there.

[mattklein123]

Prefer a loop vs. recursion.

[stevenzzzz]

Let me update the misleading comment.
This method will reschedule another cleanup task in the main thread dispatcher Q. Not exactly recursive.
We can't use a loop here as it basically means a lock/wait mechanism (on the ongoing update callbacks been finished).

[mattklein123]

Thanks this makes more sense now. Can you add a bunch more comments inside recycle and this function to describe what is going on and the various interleavings?

[mattklein123]

Ping on adding more comments.

[stevenzzzz]

oops, missed this.
I added some more comments around the function header. and also recycle.

[mattklein123]

Nothing tracks when this holder is eventually recyclable, right? Meaning, in this case it will stick around the deferred delete list until something else gets scheduled for cleanup? (Since presumably this schedule cleanup can race with the thing actually being recyclable). Maybe this is fine? Or maybe you should be checking for use count on the worker threads and scheduling cleanup if its detached and ready to be recycled (the last one)?

[stevenzzzz]

I think it's tracked:

1. A cleanup task is scheduled when we put a new item into the deferred delete queue.
2. The cleanup task will cleanup recyclable items from the Q, and reschedule another cleanup task(the recursion) if there are any non-recyclable ones.

So once an item is put into the queue, the above event-loop (1->2->2->2->....) will guarantee the items been cleaned up.

[stevenzzzz]

Stepping back though, can you refresh my memory on why this complexity is needed vs. just cancelling any potential updates before we start listener shutdown which I think would be serialized correctly? Thank you!

/wait

I think the canceling/waiting approach requires more changes to the code base. Basically, if we want to be lock free, we'd have to split the LDS update (or Server shutdown) into 2 phases:

1. set an flag or broadcast canceling signals to workers.
2. Wait to be signaled that the 'shutdown' message has been received by everyone.

In the meantime, if it's a LDS update caused ListenerImpl teardown, we have to bookkeep all the TLS resources owned by "this" ListenerImpl, and only wait on them to be cleaned up, as a RDS subscription (or any other subscription is shared between Listeners).

On the other hand, this change limit changes to tls module only, least disturbance to the code base.

[mattklein123]

Basically, if we want to be lock free, we'd have to split the LDS update (or Server shutdown) into 2 phases

To be clear, this is how it already works. See ListenerManagerImpl::drainListener which already handles draining and stopping a listener, including posting a completion back and forth. Assuming all xDS updates are stopped before removal (or drain) by definition there can be no destruction race condition. What I'm trying to get at is how hard is it really to just shut down LDS/RDS/etc. updates for a listener on the main thread before doing this? Naively it doesn't seem that hard.

[mattklein123]

I talked to @stevenzzzz offline and we are going to time box trying to implement explicit shutdown of resources prior to listener shutdown. This might be accomplished via the init manager or some similar structure. If it ends up being too hard or has complications we can come back to this.

/wait

[htuch]

I'd be in favor of not repeating something like init manager. Init manager is has turned out tobe hard to reason about, with a maze of callbacks, watchers, registration conditions, lifetime considerations and so on. History speaks for itself with all the bugs and races on this one.

I think there is a good argument to be made that the solution in this PR has benefits in that it simplifies the contract around slot ownership, e.g. the runOnAllThreads() gets the threads out of the business of worrying about tls_ slow ownership and lifetime. It also centralizes all the complexity inside of the SlotImpl, whereas init manager style solutions diffuse this complexity.

So, my $0.02 is that something like the solution in this PR is a nice direction; basically getting away from having threads worrying about TLS slots directly.

[htuch]

Throwing out another idea that has come up when chatting with @stevenzzzz; we could have a centralized (owned by Server::Instance) TLS slot manager. This will be the actual owner of the memory backing all slots and it lives forever. It would hand out slot handles and then have them returned via RAII.

[mattklein123]

So, my $0.02 is that something like the solution in this PR is a nice direction; basically getting away from having threads worrying about TLS slots directly.

I think with this solution there are going to be potentially hard to reason about thread interleavings, and I have no doubt we will have a different set of issues from this direction, though I agree it's a nice solution and it will work in the end. My reason for proposing the use of the existing init manager system is that it already exists. We must register things into the init manager, and I think it would be relatively trivial to also register a shutdown hook of some type at the same time.

With that said, I don't feel that strongly about it, so if you both want to continue with this solution we can do that.

we could have a centralized (owned by Server::Instance) TLS slot manager.

This is the way it already works?

[stevenzzzz]

Good discussion here.
Personally I am not a fan of the init-manager mechanism. It's essentially a barrier that's passed around and hard to track what's been barriered and what's(should) not.

The other problem I want to call out here is that no matter which way we go, we need to let contributors know about this problem, and we should not distributed (by capturing some shared_ptr) ownership of a tls slot or its owner to worker threads.
That has the side effect of destructing the delicate dependency graph of objects in ServerImpl and cause crash during server shutdown (IIUC, per my experience though we drain the libevent message queue when shutdown a worker, we don't prevent new callbacks been posted to Dispatcher's callback queue). An example would be post a
a callback which captures a shared_ptr to workers. On destruction, The singletonManager is destructed first, and then the RdsConfigSubscription owned by RdsRouteConfigProvider will try to remove it self from the destructed RdsConfigProviderManager.

[mattklein123]

The other problem I want to call out here is that no matter which way we go, we need to let contributors know about this problem, and we should not distributed (by capturing some shared_ptr) ownership of a tls slot or its owner to worker threads.

This is my point exactly. Although I agree Init/ShutdownManager is problematic in certain ways, its extremely explicit on what needs to happen. Basically, don't do any more updates. We are trading that for rules around how this can be used which are hard to understand and enforce. With that, I will stop there. @htuch I assume you want to move forward with this solution?

[mattklein123]

@stevenzzzz thanks for the fixes, but before we continue the review can we discuss my comment here:

Stepping back though, can you refresh my memory on why this complexity is needed vs. just cancelling any potential updates before we start listener shutdown which I think would be serialized correctly? Thank you!

I want to check and make sure we are picking the correct high level solution.

[htuch]

Can we remove tls_ as a member?

[stevenzzzz]

I am not sure I follow your question, may I ask why?
tls_ is per subscription in config-provider framework, depends on config type (delta/non-delta), it creates per-worker/x-worker-shared ConfigImpl, which is shared among providers.

[stevenzzzz]

Thx!

[stevenzzzz]

I am not sure I follow your question, may I ask why?
tls_ is per subscription in config-provider framework, depends on config type (delta/non-delta), it creates per-worker/x-worker-shared ConfigImpl, which is shared among providers.

[stevenzzzz]

Good discussion here.
Personally I am not a fan of the init-manager mechanism. It's essentially a barrier that's passed around and hard to track what's been barriered and what's(should) not.

The other problem I want to call out here is that no matter which way we go, we need to let contributors know about this problem, and we should not distributed (by capturing some shared_ptr) ownership of a tls slot or its owner to worker threads.
That has the side effect of destructing the delicate dependency graph of objects in ServerImpl and cause crash during server shutdown (IIUC, per my experience though we drain the libevent message queue when shutdown a worker, we don't prevent new callbacks been posted to Dispatcher's callback queue). An example would be post a
a callback which captures a shared_ptr to workers. On destruction, The singletonManager is destructed first, and then the RdsConfigSubscription owned by RdsRouteConfigProvider will try to remove it self from the destructed RdsConfigProviderManager.

[mattklein123]

LGTM, thanks. Flushing out a few more comments.

/wait

[mattklein123]

Thanks this makes more sense now. Can you add a bunch more comments inside recycle and this function to describe what is going on and the various interleavings?

[stevenzzzz]

/assign alyssawilk

@alyssawilk PTAL, this PR is inspired by you suggested options.

[mattklein123]

Thanks, LGTM with some remaining comments.

Also, is it possible for you to add an integration test that crashes without this fix? That would be optimal. Thank you!

/wait

[mattklein123]

Ping on adding more comments.

[mattklein123]

Thanks, LGTM modulo clang-tidy fixes and in integration test for the original problem. Thank you!

/wait

[stevenzzzz]

Thanks, LGTM with some remaining comments.

Also, is it possible for you to add an integration test that crashes without this fix? That would be optimal. Thank you!

/wait

So previously the scoped_rds_integration_test is actually flaky, it fails around 1/3000 as there was deletion of a RDS resource immediately after a update to it, but the race window is really small.
I can add a regression test that will reproduce the race, but I don't a good way to reproduce the crash in the fixed code.

[mattklein123]

LGTM, thanks for iterating here and fully discussing the solution. Will defer to @alyssawilk for further review.

[lambdai]

The Bookeeper is inspiring WRT extending the ability of Slot.

I am not sure if the SlotHolder is too complex since it is kind of re-inventing shared_ptr. What's more the Bookeeper combining with SlotHolder is duplicating the deleter of shared_ptr.

My understanding of the bookkeeper is to track

• how many runOnAllThread on the fly
• The last of Bookkeeper destruction and runOnallThread should destruct the real slot

I propose to replace SlotHolder by shared_ptr, and bookeeper should
1 ref the shared_ptr until destruction (shared_ptr as a member)
2 increase the ref of shared_ptr at runOnAllThread (no more complex than capture it)
3. choose one from 3.1 and 3.2
3.1 BookKeeper set the deleter of the shared_ptr to moving to defer delete queue
3.2 rewrite the BookKeeper so that runOnAllThread enforce a oncomplete_cb to decrease the shared_ptr on main thread. See https://github.com/envoyproxy/envoy/pull/7011/files . 3.2 doesn't even need a defer delete queue.

My proposal not decrease the complexity of SlotHolder.
With 3.2 Bookkeeper::runOnAllThread could cut the overhead and the complexity of operation on deferred deletion.

WDYT?

[lambdai]

The shared_ptr as ref_count reminds me of the pattern I eliminated in envoy before. See #7011

[alyssawilk]

I feel like this ought to be avoidable, but perhaps I'm missing something. We're using ref_count_ not as an integer, but just to have an object with a use count right?

If we made refcount_ a class, and had Bookkeeper have a reference to it (along with runOnAllThreads calls), and the refcount destructor did a dispatcher post(delete)) couldn't we avoid this extra layer of clean up complexity? We'd still delete when both the Bookkeeper and all thread references were gone, and we'd still delete in the original thread.

[stevenzzzz]

So the problem with "posting back to main-thread when the propagation is done", there again is this race-window, where the posted callback in main-thread may gets run first, and then the "copy of lambda" in the worker thread may held the last reference of the refcount_.

IMHO, We should also avoid to move the ownership of a slotImpl or its owner, by posting a callback/complete_cb which shares a portion of the object as well. That may cause complication when ServerImpl tears down, I went into a crash with that, where I posted shared_ptr back to main thread dispatcher queue, during shutdown, the SingletonManager is teared down ahead of the DispatcherImpl in ServerImpl, which includes the RouteConfigProviderManager that's referenced by the RdsRouteCnofigProvider in the dispatcher-msg-queue.

The Bookeeper is inspiring WRT extending the ability of Slot.

I am not sure if the SlotHolder is too complex since it is kind of re-inventing shared_ptr. What's more the Bookeeper combining with SlotHolder is duplicating the deleter of shared_ptr.

My understanding of the bookkeeper is to track

• how many runOnAllThread on the fly
• The last of Bookkeeper destruction and runOnallThread should destruct the real slot

I propose to replace SlotHolder by shared_ptr, and bookeeper should
1 ref the shared_ptr until destruction (shared_ptr as a member)
2 increase the ref of shared_ptr at runOnAllThread (no more complex than capture it)
3. choose one from 3.1 and 3.2
3.1 BookKeeper set the deleter of the shared_ptr to moving to defer delete queue
3.2 rewrite the BookKeeper so that runOnAllThread enforce a oncomplete_cb to decrease the shared_ptr on main thread. See https://github.com/envoyproxy/envoy/pull/7011/files . 3.2 doesn't even need a defer delete queue.

My proposal not decrease the complexity of SlotHolder.
With 3.2 Bookkeeper::runOnAllThread could cut the overhead and the complexity of operation on deferred deletion.

WDYT?

The Bookeeper is inspiring WRT extending the ability of Slot.

I am not sure if the SlotHolder is too complex since it is kind of re-inventing shared_ptr. What's more the Bookeeper combining with SlotHolder is duplicating the deleter of shared_ptr.

My understanding of the bookkeeper is to track

• how many runOnAllThread on the fly
• The last of Bookkeeper destruction and runOnallThread should destruct the real slot

I propose to replace SlotHolder by shared_ptr, and bookeeper should
1 ref the shared_ptr until destruction (shared_ptr as a member)
2 increase the ref of shared_ptr at runOnAllThread (no more complex than capture it)
3. choose one from 3.1 and 3.2
3.1 BookKeeper set the deleter of the shared_ptr to moving to defer delete queue
3.2 rewrite the BookKeeper so that runOnAllThread enforce a oncomplete_cb to decrease the shared_ptr on main thread. See https://github.com/envoyproxy/envoy/pull/7011/files . 3.2 doesn't even need a defer delete queue.

My proposal not decrease the complexity of SlotHolder.
With 3.2 Bookkeeper::runOnAllThread could cut the overhead and the complexity of operation on deferred deletion.

WDYT?

Yeah, SlotHolder is basically the obj that holds a slot and its outgoing ref-count. I made a class instead using a shared pointer as I thought it may be more "readable". the ferre

[lambdai]

Oh, SingletonManager early destruction. With this restriction I don't have a good opinion...

[stevenzzzz]

Talked w/ Alyssa offline, get the idea now. I believe this matches what lamndai suggested as well.
The "schedule a cleanup callback" logic is removed and each Slot Bookkeeper fires a cleanup callback on the ref-count's destruction.

[alyssawilk]

I'm not clear on why we need this. Can't we have both Bookkeeper and threads have pointers to ref_count_, and have scheduleCleanup either straight up delete slot or schedule deletion via post? Why do we need the intermediate map?

[stevenzzzz]

when we destruct the Bookkeeper, we can't delete the SlotImpl yet if the ref_count > 1, we need to detach the SlotImpl instance from its owner, and wait for the ref_count to go to 0 and make the post-call.

[stevenzzzz]

We talked offline, Alyssa suggested to remove the intermediate storage (deferred_deletes_ ) as well, IIUC, by: releasing the SlotImpl from the unique_ptr, having the ref_count shared_ptr post a callback on destruction to the Main dispatcher. There is a risk of leaking at shutdown time though: in Server::InstanceImpl::terminate(), workers are stopped after main dispatcher loop is exited, if a worker posts a cleanup callback, the callback will not be run, the SlotImpl instance pointed to by the raw pointer will be leaked. So the intermediate container is kinda unavoidable.

[alyssawilk]

LGTM modulo the spelling error.

As an aside, I think we need to do some infrastructure work to make the cross-thread deletion need less rigmarole, but I think that's outside of the scope of this PR. Thanks for all the cleanup you were willing to do!

[stevenzzzz]

/wait