Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: native test wrapper waits for all grandchild processes #8005

Closed
laszlocsomor opened this issue Apr 11, 2019 · 4 comments
Closed

Windows: native test wrapper waits for all grandchild processes #8005

laszlocsomor opened this issue Apr 11, 2019 · 4 comments
Assignees
Labels
area-Windows Windows-specific issues and feature requests P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug

Comments

@laszlocsomor
Copy link
Contributor

Description of the problem / feature request:

The Windows-native test wrapper waits for all subprocesses of the test to terminate, even after the test itself terminated.

I believe the test wrapper should use job objects:

HANDLE job = CreateJobObject(NULL, NULL);

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

BUILD:

java_test(
    name = "sleepy",
    main_class = "Sleepy",
    srcs = ["Sleepy.java"],
    timeout = "short",
    use_testrunner = 0,
)

Sleepy.java:

public class Sleepy {
  public static void main(String[] args) throws Exception {
    new ProcessBuilder("C:/msys64/usr/bin/sleep.exe", "15").inheritIO().start();
  }
}

Without test wrapper:

C:\src\tmp>c:\src\bazel\bazel-bin\src\bazel.exe test -t- --noincompatible_windows_native_test_wrapper //:sleepy
(...)
//:sleepy                                                                PASSED in 1.8s

With test wrapper:

C:\src\tmp>c:\src\bazel\bazel-bin\src\bazel.exe test -t- --incompatible_windows_native_test_wrapper //:sleepy
(...)
//:sleepy                                                                PASSED in 15.7s

INFO: Build completed successfully, 2 total actions

What operating system are you running Bazel on?

Windows 10

If bazel info release returns "development version" or "(@non-git)", tell us how you built Bazel.

Built from source at c6c3030

@laszlocsomor laszlocsomor self-assigned this Apr 11, 2019
@laszlocsomor laszlocsomor added bazel 1.0 P2 We'll consider working on this in future. (Assignee optional) area-Windows Windows-specific issues and feature requests type: bug labels Apr 11, 2019
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 15, 2019
Refactor the Windows JNI library:

- create process.cc and process.h
- create a WaitableProcess class
- move the process creation, waiting, and
  termination logic to WaitableProcess

Next step: use WaitableProcess in the
Windows-native test wrapper, to let it kill the
subprocess tree and fix bazelbuild#8005
laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Apr 17, 2019
The Windows-native test wrapper now uses the JNI
library's WaitableProcess to create, wait for, and
terminate the subprocess.

This allows killing the whole subprocess tree and
not waiting for sub-subprocesses.

Fixes bazelbuild#8005
@jmmv
Copy link
Contributor

jmmv commented Nov 15, 2019

Mostly out of curiosity because I was filing #10245 and this bug turned out as suggested.

How was this fixed? If I'm reading things correctly, Bazel now kills the process group on Windows, right? But do you wait for the subprocesses at all? If not, you'd be subject to the same issues I reported.

@laszlocsomor
Copy link
Contributor Author

If a test times out, Bazel terminates the process tree. We don't wait for subprocesses in this case, so they could leave stale state behind.

@laszlocsomor
Copy link
Contributor Author

As best as I know, processes can't signal each other on Windows as they can on Linux, so there's no generic graceful termination mechanism like SIGINT or SIGTERM, just the blunt TerminateProcess (like SIGKILL).

PostQuitMessage comes to mind for windowed applications (which the majority of Bazel-ran tools aren't), but that requires cooperation from the process (running a message processing loop).

@jmmv
Copy link
Contributor

jmmv commented Nov 15, 2019

Note that the process-wrapper on Unix systems doesn't allow processes to terminate gracefully either, in the general case: it just sends everything a SIGKILL. So what you are doing on Windows seems very similar.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Windows Windows-specific issues and feature requests P2 We'll consider working on this in future. (Assignee optional) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: bug
Projects
None yet
3 participants