Fix a deadlock in Session.Close() during control connection reconnect #1688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added the test mentioned in #1687 to this branch (locally), ran it and it passed multiple times. It consistently gets stuck when I run it in |
We switched from separate mutex for closing to sessionStateMu in 312a614. This change introduced a deadlock. We don't need to hold the mutex for the whole duration of Close(), we only need to update the status atomically. Previously IsClosed() returned true only after all closing is done (because of the deferred unlock). We can emulate that by setting isClosed at the end of Close(). We need a new variable to ensure that Close() is only executed once. Fixes apache#1687
martin-sucha
force-pushed
the
ms/issue-1687
branch
from
c987c28 to
b8c4e16
Compare
|
Thanks for checking! I've noticed missing unlock in the return branch, so I added it. |
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.
We switched from separate mutex for closing to sessionStateMu in 312a614.
This change introduced a deadlock.
We don't need to hold the mutex for the whole duration of Close(), we only need to update the status atomically.
Previously IsClosed() returned true only after all closing is done (because of the deferred unlock).
We can emulate that by setting isClosed at the end of Close(). We need a new variable to ensure that Close() is only executed once.
Fixes #1687