Fix a race condition in Bazel's Windows process management. #6860
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
philwo
commented
Dec 7, 2018
src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
Show resolved
Hide resolved
|
Can we add a test to prevent #6853? |
laszlocsomor
reviewed
Dec 7, 2018
laszlocsomor
suggested changes
Dec 7, 2018
src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
Show resolved
Hide resolved
|
Thanks so much for the good comments :) I've addressed them. |
|
I will add a test as requested by @meteorcloudy, too. |
laszlocsomor
suggested changes
Jan 15, 2019
src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocess.java
Outdated
Show resolved
Hide resolved
|
After discussing with @philwo in person: let's address the stylistic comments in a follow-up PR and only focus on the technical one. |
Move deletion logic into destructor.
This allows us to safely close the process handle early, while still allowing Bazel to ask for the exit code later.
If we don't have job objects, we terminate the process itself, but this could fail in case the process has already exited itself at this point. Check for this condition and also wait for the termination to succeed to guarantee that the exit code can be safely queried afterwards.
While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit. This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI: https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303 "Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied." This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing. Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743
…ng the error message.
|
I noticed that we already have the sleepy test in our tests: test_test_timeout in bazel_test_test.sh. |
laszlocsomor
approved these changes
Jan 15, 2019
|
Should we cherry-pick the fix into 0.22? |
I think it's too risky given the size of the change and also it's technically not a regression... :| |
|
OK, the good thing is we don't see the Windows flaky error as often as before with 0.21.0. So I'm fine waiting for 0.23 |
aehlig
pushed a commit
that referenced
this pull request
Jan 18, 2019
(Second version of this PR that fixes the issues from the last time and refactors the code a bit to make it easier to understand and maintain.) While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit. This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI: https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303 "Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied." This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing. Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743 Closes #6860. PiperOrigin-RevId: 229371044
aehlig
pushed a commit
that referenced
this pull request
Jan 23, 2019
(Second version of this PR that fixes the issues from the last time and refactors the code a bit to make it easier to understand and maintain.) While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit. This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI: https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303 "Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied." This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing. Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743 Closes #6860. PiperOrigin-RevId: 229371044
aehlig
pushed a commit
that referenced
this pull request
Jan 23, 2019
(Second version of this PR that fixes the issues from the last time and refactors the code a bit to make it easier to understand and maintain.) While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit. This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI: https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303 "Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied." This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing. Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743 Closes #6860. PiperOrigin-RevId: 229371044
weixiao-huang
pushed a commit
to weixiao-huang/bazel
that referenced
this pull request
Jan 31, 2019
(Second version of this PR that fixes the issues from the last time and refactors the code a bit to make it easier to understand and maintain.) While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit. This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI: https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303 "Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied." This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing. Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743 Closes bazelbuild#6860. PiperOrigin-RevId: 229371044
philwo
added
the
team-OSS
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.
(Second version of this PR that fixes the issues from the last time and refactors the code a bit to make it easier to understand and maintain.)
While we did correctly terminate the job object of a process when calling Subprocess.destroy and we did correctly wait for the process itself to exit in Subprocess.waitFor, we didn't wait for its children to exit.
This is usually not a big problem, except when they keep open some file handles to files that Bazel immediately tris to delete after killing / waiting for the main process, in which case the files might still be in use. This is the reason why we're seeing errors like this on CI:
https://buildkite.com/bazel/bazel-bazel/builds/5774#27afa2c9-9ff1-4224-9df7-ba0253e7e317/193-303
"Caught I/O exception: Cannot delete path (something below $TEST_TMPDIR): Access is denied."
This CL modifies the behavior to explicitly terminate the job object in Subprocess.waitFor when the main process has exited and then wait for all transitive subprocesses to exit, before continuing.
Due to some interesting semantics, one cannot simply use WaitForSingleObject on the Job handle, instead one has to use an IoCompletionPort that gets a signal when the last job in the group has exited: https://blogs.msdn.microsoft.com/oldnewthing/20130405-00/?p=4743