fix(servstate): don't hold both servicesLock and state lock at once #359
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
This avoids the 3-lock deadlock described in canonical#314. Other goroutines may be holding the state lock and waiting for the services lock, so it's problematic to acquire both locks at once. Break that part of the cycle. We could do this inside serviceForStart/serviceForStop by manually calling Unlock() sooner, but that's error-prone, so continue using defer, but have the caller write the task log (which needs the state lock) after the services lock is released.
hpidcock
approved these changes
Feb 14, 2024
There was a problem hiding this comment.
This feels like a reasonable compromise, but I'd like to see something like #356 actually solve the problem. The nature of logging is that it can occur in a variety of places, and as you can see here, it is not the most convenient of things to pass around.
flotter
approved these changes
Feb 14, 2024
Closed
This test consistently FAILs without the fix in this PR, but consistently PASSes with the fix in this PR. The repro is basically as per the instructions at canonical#314 (comment)
hpidcock
approved these changes
Feb 15, 2024
benhoyt
added a commit
that referenced
this pull request
Feb 15, 2024
…359) * fix(servstate): don't hold both servicesLock and state lock at once This avoids the 3-lock deadlock described in #314. Other goroutines may be holding the state lock and waiting for the services lock, so it's problematic to acquire both locks at once. Break that part of the cycle. We could do this inside serviceForStart/serviceForStop by manually calling Unlock() sooner, but that's error-prone, so continue using defer, but have the caller write the task log (which needs the state lock) after the services lock is released. * Add regression test for the deadlock issue This test consistently FAILs without the fix in this PR, but consistently PASSes with the fix in this PR. The repro is basically as per the instructions at #314 (comment)
benhoyt
added a commit
to benhoyt/pebble
that referenced
this pull request
Feb 20, 2024
…anonical#359) * fix(servstate): don't hold both servicesLock and state lock at once This avoids the 3-lock deadlock described in canonical#314. Other goroutines may be holding the state lock and waiting for the services lock, so it's problematic to acquire both locks at once. Break that part of the cycle. We could do this inside serviceForStart/serviceForStop by manually calling Unlock() sooner, but that's error-prone, so continue using defer, but have the caller write the task log (which needs the state lock) after the services lock is released. * Add regression test for the deadlock issue This test consistently FAILs without the fix in this PR, but consistently PASSes with the fix in this PR. The repro is basically as per the instructions at canonical#314 (comment)
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.
This avoids the 3-lock deadlock described in
#314. Other goroutines
may be holding the state lock and waiting for the services lock, so
it's problematic to acquire both locks at once. Break that part of the cycle.
We could do this inside serviceForStart/serviceForStop by manually
calling Unlock() sooner, but that's error-prone, so continue using defer,
but have the caller write the task log (which needs the state lock) after
the services lock is released.
This is in preference to the more invasive change in #356