<EC>: perhaps fixed race conditions and problems when creating and terminating process groups on Win32

* Sometimes already the job assignment failed in CI builds.
  We now check whether the process is still alive before throwing an error.
* We now set the JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE flag.
  This theoretically shouldn't be necessary when using TerminateJobObject(),
  but who knows.

3 files changed, 35 insertions, 4 deletions

diff --git a/TODO b/TODO
index d4fd137..e9bf2ac 100644
--- a/TODO
+++ b/TODO
@@ -20,6 +20,7 @@ Known Bugs:
    Could this be a race condition when adding the process to the job object?
    Perhaps the child process should be created suspended before being added
    to the job object. Glib does not currently allow that.
+   This could already be fixed.
  * dlmalloc's malloc_trim() does not seem to free any resident memory
    after hitting the OOM limit, eg. after <%a>.
    Apparently an effect of HAVE_MORECORE (sbrk()) - some allocation is
diff --git a/src/error.h b/src/error.h
index 469d957..5ffd434 100644
--- a/src/error.h
+++ b/src/error.h
@@ -50,6 +50,7 @@ typedef enum {
 	TECO_ERROR_EDITINGLOCALQREG,
 	TECO_ERROR_MEMLIMIT,
 	TECO_ERROR_CLIPBOARD,
+	TECO_ERROR_WIN32,
 
 	/** Interrupt current operation */
 	TECO_ERROR_INTERRUPTED,
@@ -138,6 +139,15 @@ teco_error_editinglocalqreg_set(GError **error, const gchar *name, gsize len)
 	            "Editing local Q-Register \"%s\" at end of macro call", name_printable);
 }
 
+#ifdef G_OS_WIN32
+static inline void
+teco_error_win32_set(GError **error, const gchar *prefix, gint err)
+{
+	g_autofree gchar *msg = g_win32_error_message(err);
+	g_set_error(error, TECO_ERROR, TECO_ERROR_WIN32, "%s: %s", prefix, msg);
+}
+#endif
+
 static inline void
 teco_error_interrupted_set(GError **error)
 {
diff --git a/src/spawn.c b/src/spawn.c
index e6d620c..a1b8ac8 100644
--- a/src/spawn.c
+++ b/src/spawn.c
@@ -294,15 +294,35 @@ teco_state_execute_done(teco_machine_main_t *ctx, const teco_string_t *str, GErr
 	teco_spawn_ctx.interrupted = FALSE;
 
 #ifdef G_OS_WIN32
+	teco_spawn_ctx.pid = CreateJobObject(NULL, NULL);
+	if (!teco_spawn_ctx.pid) {
+		teco_error_win32_set(error, "Cannot create job object", GetLastError());
+		goto gerror;
+	}
+	JOBOBJECT_EXTENDED_LIMIT_INFORMATION job_info = {
+		.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE
+	};
+	if (!SetInformationJobObject(teco_spawn_ctx.pid, JobObjectExtendedLimitInformation,
+	                             &job_info, sizeof(job_info))) {
+		CloseHandle(teco_spawn_ctx.pid);
+		teco_error_win32_set(error, "Cannot configure job object", GetLastError());
+		goto gerror;
+	}
 	/*
 	 * Assigning the process to a job object will allow us to
 	 * kill the entire process tree relatively easily and without
 	 * race conditions.
+	 * There can however be a race condition while assigning the
+	 * job object since the process could already be dead.
 	 */
-	teco_spawn_ctx.pid = CreateJobObjectA(NULL, NULL);
-	if (!teco_spawn_ctx.pid || !AssignProcessToJobObject(teco_spawn_ctx.pid, pid)) {
-		g_set_error(error, TECO_ERROR, TECO_ERROR_FAILED,
-		            "Cannot assign process to job object (%lu)", GetLastError());
+	DWORD exit_code;
+	if (!AssignProcessToJobObject(teco_spawn_ctx.pid, pid) &&
+	    (GetLastError() != ERROR_ACCESS_DENIED ||
+	     !GetExitCodeProcess(teco_spawn_ctx.pid, &exit_code) ||
+	     exit_code == STILL_ACTIVE)) {
+		CloseHandle(teco_spawn_ctx.pid);
+		teco_error_win32_set(error, "Cannot assign process to job object",
+		                     GetLastError());
 		goto gerror;
 	}