Repository: bazelbuild/bazel
Issue: Windows: native test wrapper waits for all grandchild processes
Labels: type: bug
P2
area-Windows
team-OSS

### 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: https://github.com/bazelbuild/bazel/blob/c6c303009c207dae2e6342eee9b8a25e37ca20ca/src/main/native/windows/processes-jni.cc#L370

### 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 https://github.com/bazelbuild/bazel/commit/c6c303009c207dae2e6342eee9b8a25e37ca20ca

Issue comments:
Mostly out of curiosity because I was filing https://github.com/bazelbuild/bazel/issues/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.
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.
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).
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.

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/native/windows/process.cc b/src/main/native/windows/process.cc
index 82fec19a2150cc..5b7976add1a9f2 100644
--- a/src/main/native/windows/process.cc
+++ b/src/main/native/windows/process.cc
@@ -33,6 +33,7 @@ bool WaitableProcess::Create(const std::wstring& argv0,
                              const std::wstring& argv_rest, void* env,
                              const std::wstring& wcwd, HANDLE stdin_process,
                              HANDLE stdout_process, HANDLE stderr_process,
+                             LARGE_INTEGER* opt_out_start_time,
                              std::wstring* error) {
   std::wstring cwd;
   std::wstring error_msg(AsShortPath(wcwd, &cwd));
@@ -177,11 +178,22 @@ bool WaitableProcess::Create(const std::wstring& argv0,
     return false;
   }
 
+  if (opt_out_start_time) {
+    QueryPerformanceCounter(opt_out_start_time);
+  }
   *error = L"";
   return true;
 }
 
-int WaitableProcess::WaitFor(int64_t timeout_msec, std::wstring* error) {
+int WaitableProcess::WaitFor(
+    int64_t timeout_msec, LARGE_INTEGER* opt_out_end_time,
+    std::wstring* error) {
+  struct Defer {
+    LARGE_INTEGER* t;
+    Defer(LARGE_INTEGER* cnt) : t(cnt) {}
+    ~Defer() { if (t) { QueryPerformanceCounter(t); } }
+  } defer_query_end_time(opt_out_end_time);
+
   DWORD win32_timeout = timeout_msec < 0 ? INFINITE : timeout_msec;
   int result;
   switch (WaitForSingleObject(process_, win32_timeout)) {
diff --git a/src/main/native/windows/process.h b/src/main/native/windows/process.h
index e750f465608bfc..de0e1e4fda3888 100644
--- a/src/main/native/windows/process.h
+++ b/src/main/native/windows/process.h
@@ -42,9 +42,10 @@ class WaitableProcess {
   bool Create(const std::wstring& argv0, const std::wstring& argv_rest,
               void* env, const std::wstring& wcwd, HANDLE stdin_process,
               HANDLE stdout_process, HANDLE stderr_process,
-              std::wstring* error);
+              LARGE_INTEGER* opt_out_start_time, std::wstring* error);
 
-  int WaitFor(int64_t timeout_msec, std::wstring* error);
+  int WaitFor(int64_t timeout_msec, LARGE_INTEGER* opt_out_end_time,
+              std::wstring* error);
 
   int GetExitCode(std::wstring* error);
 
diff --git a/src/main/native/windows/processes-jni.cc b/src/main/native/windows/processes-jni.cc
index e3bb480791a1de..f58397e2a2be2d 100644
--- a/src/main/native/windows/processes-jni.cc
+++ b/src/main/native/windows/processes-jni.cc
@@ -314,7 +314,7 @@ class NativeProcess {
     return proc_.Create(
         wpath, bazel::windows::GetJavaWstring(env, java_argv_rest),
         env_map.ptr(), bazel::windows::GetJavaWpath(env, java_cwd),
-        stdin_process, stdout_process, stderr_process, &error_);
+        stdin_process, stdout_process, stderr_process, nullptr, &error_);
   }
 
   void CloseStdin() {
@@ -325,7 +325,7 @@ class NativeProcess {
 
   // Wait for this process to exit (or timeout).
   int WaitFor(int64_t timeout_msec) {
-    return proc_.WaitFor(timeout_msec, &error_);
+    return proc_.WaitFor(timeout_msec, nullptr, &error_);
   }
 
   // Returns the exit code of the process if it has already exited. If the


Test patch:
diff --git a/tools/test/BUILD b/tools/test/BUILD
index 6deb7dde275004..d259336de9c535 100644
--- a/tools/test/BUILD
+++ b/tools/test/BUILD
@@ -79,6 +79,7 @@ cc_library(
             "//src/main/cpp/util:filesystem",
             "//src/main/cpp/util:strings",
             "//src/main/native/windows:lib-file",
+            "//src/main/native/windows:lib-process",
             "//src/main/native/windows:lib-util",
             "//src/tools/launcher/util",
             "//third_party/ijar:zip",
diff --git a/tools/test/windows/tw.cc b/tools/test/windows/tw.cc
index d65312bc3dae58..ac9eab288410b1 100644
--- a/tools/test/windows/tw.cc
+++ b/tools/test/windows/tw.cc
@@ -42,6 +42,7 @@
 #include "src/main/cpp/util/path_platform.h"
 #include "src/main/cpp/util/strings.h"
 #include "src/main/native/windows/file.h"
+#include "src/main/native/windows/process.h"
 #include "src/main/native/windows/util.h"
 #include "src/tools/launcher/util/launcher_util.h"
 #include "third_party/ijar/common.h"
@@ -1098,7 +1099,7 @@ bool CreateCommandLine(const Path& path, const std::wstring& args,
 bool StartSubprocess(const Path& path, const std::wstring& args,
                      const Path& outerr, std::unique_ptr<Tee>* tee,
                      LARGE_INTEGER* start_time,
-                     bazel::windows::AutoHandle* process) {
+                     bazel::windows::WaitableProcess* process) {
   SECURITY_ATTRIBUTES inheritable_handle_sa = {sizeof(SECURITY_ATTRIBUTES),
                                                NULL, TRUE};
 
@@ -1139,16 +1140,6 @@ bool StartSubprocess(const Path& path, const std::wstring& args,
     return false;
   }
 
-  // Create an attribute object that specifies which particular handles shall
-  // the subprocess inherit. We pass this object to CreateProcessW.
-  std::unique_ptr<bazel::windows::AutoAttributeList> attr_list;
-  std::wstring werror;
-  if (!bazel::windows::AutoAttributeList::Create(
-          devnull_read, pipe_write, pipe_write_dup, &attr_list, &werror)) {
-    LogError(__LINE__, werror);
-    return false;
-  }
-
   // Open a handle to the test log file. The "tee" thread will write everything
   // into it that the subprocess writes to the pipe.
   bazel::windows::AutoHandle test_outerr;
@@ -1177,57 +1168,13 @@ bool StartSubprocess(const Path& path, const std::wstring& args,
     return false;
   }
 
-  PROCESS_INFORMATION process_info;
-  STARTUPINFOEXW startup_info;
-  attr_list->InitStartupInfoExW(&startup_info);
-
-  std::unique_ptr<WCHAR[]> cmdline;
-  if (!CreateCommandLine(path, args, &cmdline)) {
-    return false;
-  }
-
-  QueryPerformanceCounter(start_time);
-  if (CreateProcessW(NULL, cmdline.get(), NULL, NULL, TRUE,
-                     CREATE_UNICODE_ENVIRONMENT | EXTENDED_STARTUPINFO_PRESENT,
-                     NULL, NULL, &startup_info.StartupInfo,
-                     &process_info) != 0) {
-    CloseHandle(process_info.hThread);
-    *process = process_info.hProcess;
-    return true;
-  } else {
-    DWORD err = GetLastError();
-    LogErrorWithValue(
-        __LINE__,
-        (std::wstring(L"CreateProcessW failed (") + cmdline.get() + L")")
-            .c_str(),
-        err);
+  std::wstring werror;
+  if (!process->Create(path.Get(), args, nullptr, L"", devnull_read, pipe_write,
+                       pipe_write_dup, start_time, &werror)) {
+    LogError(__LINE__, werror);
     return false;
   }
-}
-
-int WaitForSubprocess(HANDLE process, LARGE_INTEGER* end_time) {
-  DWORD result = WaitForSingleObject(process, INFINITE);
-  QueryPerformanceCounter(end_time);
-  switch (result) {
-    case WAIT_OBJECT_0: {
-      DWORD exit_code;
-      if (!GetExitCodeProcess(process, &exit_code)) {
-        DWORD err = GetLastError();
-        LogErrorWithValue(__LINE__, "GetExitCodeProcess failed", err);
-        return 1;
-      }
-      return exit_code;
-    }
-    case WAIT_FAILED: {
-      DWORD err = GetLastError();
-      LogErrorWithValue(__LINE__, "WaitForSingleObject failed", err);
-      return 1;
-    }
-    default:
-      LogErrorWithValue(
-          __LINE__, "WaitForSingleObject returned unexpected result", result);
-      return 1;
-  }
+  return true;
 }
 
 bool ArchiveUndeclaredOutputs(const UndeclaredOutputs& undecl) {
@@ -1385,14 +1332,27 @@ bool TeeImpl::MainFunc() const {
 int RunSubprocess(const Path& test_path, const std::wstring& args,
                   const Path& test_outerr, Duration* test_duration) {
   std::unique_ptr<Tee> tee;
-  bazel::windows::AutoHandle process;
+  bazel::windows::WaitableProcess process;
   LARGE_INTEGER start, end, freq;
   if (!StartSubprocess(test_path, args, test_outerr, &tee, &start, &process)) {
     LogError(__LINE__, std::wstring(L"Failed to start test process \"") +
                            test_path.Get() + L"\"");
     return 1;
   }
-  int result = WaitForSubprocess(process, &end);
+
+  std::wstring werror;
+  int wait_res = process.WaitFor(-1, &end, &werror);
+  if (wait_res != bazel::windows::WaitableProcess::kWaitSuccess) {
+    LogErrorWithValue(__LINE__, werror, wait_res);
+    return 1;
+  }
+
+  werror.clear();
+  int result = process.GetExitCode(&werror);
+  if (!werror.empty()) {
+    LogError(__LINE__, werror);
+    return 1;
+  }
 
   QueryPerformanceFrequency(&freq);
   end.QuadPart -= start.QuadPart;