[Merged by Bors] - Remove double-locking deadlock from HTTP API #4687
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
michaelsproul
added
bug
michaelsproul
force-pushed
the
fix-api-deadlock
branch
from
a27ac1e to
33c1cfa
Compare
jimmygchen
added
ready-for-merge
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Aug 31, 2023
## Issue Addressed Fix a deadlock introduced in #4236 which was caught during the v4.4.0 release testing cycle (with thanks to @paulhauner and `gdb`). ## Proposed Changes Avoid re-locking the fork choice read lock when querying a state by root in the HTTP API. This avoids a deadlock due to the lock already being held. ## Additional Info The [RwLock docs](https://docs.rs/lock_api/latest/lock_api/struct.RwLock.html#method.read) explicitly advise against re-locking: > Note that attempts to recursively acquire a read lock on a RwLock when the current thread already holds one may result in a deadlock.
|
Pull request successfully merged into unstable. Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Sep 4, 2023
## Proposed Changes New release to replace the cancelled v4.4.0 release. This release includes the bugfix #4687 which avoids a deadlock that was present in v4.4.0. ## Additional Info Awaiting testing over the weekend this will be merged Monday September 4th.
bors bot
pushed a commit
that referenced
this pull request
Sep 15, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
bors bot
pushed a commit
that referenced
this pull request
Sep 15, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
bors bot
pushed a commit
that referenced
this pull request
Sep 15, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
bors bot
pushed a commit
that referenced
this pull request
Sep 20, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
bors bot
pushed a commit
that referenced
this pull request
Sep 21, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
bors bot
pushed a commit
that referenced
this pull request
Sep 21, 2023
…4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in #4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
…igp#4688) ## Issue Addressed I went through the code base and look for places where we acquire fork choice locks (after the deadlock bug was found and fixed in sigp#4687), and discovered an instance where we re-acquire a lock immediately after dropping it. This shouldn't cause deadlock like the other issue, but is slightly less efficient.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed Fix a deadlock introduced in sigp#4236 which was caught during the v4.4.0 release testing cycle (with thanks to @paulhauner and `gdb`). ## Proposed Changes Avoid re-locking the fork choice read lock when querying a state by root in the HTTP API. This avoids a deadlock due to the lock already being held. ## Additional Info The [RwLock docs](https://docs.rs/lock_api/latest/lock_api/struct.RwLock.html#method.read) explicitly advise against re-locking: > Note that attempts to recursively acquire a read lock on a RwLock when the current thread already holds one may result in a deadlock.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
New release to replace the cancelled v4.4.0 release. This release includes the bugfix sigp#4687 which avoids a deadlock that was present in v4.4.0. Awaiting testing over the weekend this will be merged Monday September 4th.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
## Issue Addressed Fix a deadlock introduced in sigp#4236 which was caught during the v4.4.0 release testing cycle (with thanks to @paulhauner and `gdb`). ## Proposed Changes Avoid re-locking the fork choice read lock when querying a state by root in the HTTP API. This avoids a deadlock due to the lock already being held. ## Additional Info The [RwLock docs](https://docs.rs/lock_api/latest/lock_api/struct.RwLock.html#method.read) explicitly advise against re-locking: > Note that attempts to recursively acquire a read lock on a RwLock when the current thread already holds one may result in a deadlock.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
New release to replace the cancelled v4.4.0 release. This release includes the bugfix sigp#4687 which avoids a deadlock that was present in v4.4.0. Awaiting testing over the weekend this will be merged Monday September 4th.
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.
Issue Addressed
Fix a deadlock introduced in #4236 which was caught during the v4.4.0 release testing cycle (with thanks to @paulhauner and
gdb).Proposed Changes
Avoid re-locking the fork choice read lock when querying a state by root in the HTTP API. This avoids a deadlock due to the lock already being held.
Additional Info
The RwLock docs explicitly advise against re-locking: