fix: avoid deadlocks during shutdown #1397
Conversation
Avoid deadlocks caused by the SessionPool both controlling the lifetime of the background threads in its destructor, and being also destructed by those threads. The background threads are now owned by the ConnectionImpl.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1397 +/- ##
==========================================
- Coverage 94.38% 94.35% -0.03%
==========================================
Files 190 190
Lines 15659 15679 +20
==========================================
+ Hits 14779 14794 +15
- Misses 880 885 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'd considered this but I didn't think it was viable since ConnectionImpl doesn't actually own the SessionPool, it just has a shared_ptr to it. So, I don't think the order of operations you gave in chat is guaranteed - the SessionPool may outlive the ConnectionImpl, although it's possible that doesn't happen in any of our tests.
But, the one twist here I hadn't considered was passing CompletionQueue to the SessionPool instead of a pointer to BackgroundThreads. If we did the latter then CompletionQueue could end up dereferencing freed memory (we can't fix that with shared_ptr<BackgroundThreads> without running into the original issue). But, in this case, I believe we might end up with a CompletionQueue with no threads servicing it, is that ok? I recall having issues with that in the past.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @devbww, @mr-salty, and @scotthart)
There was a problem hiding this comment.
I'd considered this but I didn't think it was viable since
ConnectionImpldoesn't actually own theSessionPool, it just has ashared_ptrto it. So, I don't think the order of operations you gave in chat is guaranteed - theSessionPoolmay outlive theConnectionImpl,
Good point.
although it's possible that doesn't happen in any of our tests.
I would argue that none of the callbacks inside SessionPool can block trying to join the threads for the simple reason that they never had access to the BackgroundThreads object in the first place.
But, the one twist here I hadn't considered was passing
CompletionQueueto theSessionPoolinstead of a pointer toBackgroundThreads. If we did the latter thenCompletionQueuecould end up dereferencing freed memory (we can't fix that withshared_ptr<BackgroundThreads>without running into the original issue).
Sure, but we don't do that...
But, in this case, I believe we might end up with a
CompletionQueuewith no threads servicing it, is that ok?
During shutdown? I think it is. I mean, sure, we still may want to add the SessionPool::Shutdown() function to wait for all the timers and RPCs to finish, but this will not deadlock with or without that function.
I recall having issues with that in the past.
There is the thing in **grpc::**CompletionQueue having to wait for all the active operations, but we fixed that in the shutdown for **google::cloud::**CompletionQueue
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @devbww and @scotthart)
There was a problem hiding this comment.
I'm still a bit concerned about the possibility of a unserviced CompletionQueue but I'm not sure how to tease that out... and in any case this is an improvement over the current situation and will unblock me - and I can see if any issues arise with my pending changes.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @devbww and @scotthart)
|
I am happy to write the |
…nner#1397) Avoid deadlocks caused by the SessionPool both controlling the lifetime of the background threads in its destructor, and being also destructed by those threads. The background threads are now owned by the ConnectionImpl.
Avoid deadlocks caused by the SessionPool both controlling the
lifetime of the background threads in its destructor, and being also
destructed by those threads. The background threads are now owned by
the ConnectionImpl.
This change is