Revert switch of env locking to rwlock, to fix deadlock in process spawning #82877
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
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ rollup=never |
|
📌 Commit acdca31 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r+ Thanks for investigating! Let's open an issue to check out the underlying problem that's probably still there with a Mutex. Once we fix that part, we can probably put the rwlock back. (Not unlocking in the child by mem::forget'ing the guard might already be enough.) |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit acdca31 has been approved by |
|
Oh. Review race condition. |
|
@bors p=1 To avoid further CI timeouts. |
Filed #82879 to track this. |
|
⌛ Testing commit acdca31 with merge 970337fe14c426faa0bf26e21a4038065531ca59... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
I saw this failure on #82847 (comment), spurious? |
bors
added
S-waiting-on-bors
|
⌛ Testing commit acdca31 with merge f6fb3c23628c09c43b485e0d41578923e14af5a5... |
|
@JohnTitor I can reproduce that error, so I filed #82886. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit acdca31 with merge f874496963709b086adf4a337be49ea6ff959119... |
|
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
⌛ Testing commit acdca31 with merge 81b67ade8411474035c922911d623345fadbd59f... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
@bors p=20 (To make it go in front of rollups.) |
|
☀️ Test successful - checks-actions |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 15, 2021
use RWlock when accessing os::env (take 2) This reverts commit acdca31 (rust-lang#82877) i.e. redoes rust-lang#81850 since the invalid unlock attempts in the child process have been fixed in rust-lang#82949 r? `@joshtriplett`
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 reverts commit 354f19c, reversing changes made to 0cfba2f.
PR #81850 switched the environment lock from a mutex to an rwlock. However, process spawning (when not able to use
posix_spawn) locks the environment before forking, and unlocks it after forking (in both the parent and the child). With a mutex, this works (although probably not correct even with a mutex). With an rwlock, on at least some targets, unlocking in the child does not work correctly, resulting in a deadlock.This has manifested as CI hangs on i686 Linux; that target doesn't use
posix_spawnin the CI environment due to the age of the installed C library (currently glibc 2.23). (Switching toposix_spawnwould just mask this issue, though, which would still arise in any case that can't useposix_spawn.)Some additional cleanup of environment handling around process spawning may help, but for now, revert the PR and go back to a standard mutex.
Fixes #82221