nonblock call polishing in pio

lib/pio.c

@@ -601,9 +601,17 @@ static hawk_pio_pid_t standard_fork_and_exec (hawk_pio_t* pio, int pipes[], para
 
 #endif
 
+#if !defined(__DOS__)
+/* not needed on DOS: hawk_pio_init() bails out with HAWK_ENOIMPL there before
+ * any pipe is ever created, so there is no call site for this. */
 static int set_pipe_nonblock (hawk_pio_t* pio, hawk_pio_hnd_t fd, int enabled)
 {
-#if defined(O_NONBLOCK)
+#if defined(_WIN32)
+	/* anonymous pipes created by CreatePipe() cannot be switched to
+	 * non-blocking mode. */
+	hawk_gem_seterrnum(pio->gem, HAWK_NULL, HAWK_ENOIMPL);
+	return -1;
+#elif defined(O_NONBLOCK)
 	int flag = HAWK_FCNTL(fd, F_GETFL, 0);
 	if (flag >= 0) flag = HAWK_FCNTL(fd, F_SETFL, (enabled? (flag | O_NONBLOCK): (flag & ~O_NONBLOCK)));
 	if (flag <= -1) hawk_gem_seterrnum(pio->gem, HAWK_NULL, hawk_syserr_to_errnum(errno));
@@ -613,6 +621,7 @@ static int set_pipe_nonblock (hawk_pio_t* pio, hawk_pio_hnd_t fd, int enabled)
 	return -1;
 #endif
 }
+#endif
 
 
 int hawk_pio_init (hawk_pio_t* pio, hawk_gem_t* gem, const hawk_ooch_t* cmd, int flags, hawk_pio_env_mk_t env_mk, void* env_ctx)
@@ -725,6 +734,12 @@ int hawk_pio_init (hawk_pio_t* pio, hawk_gem_t* gem, const hawk_ooch_t* cmd, int
 			}
 		}
 
+		/* handle[1] is the parent's end and is never inherited by the child,
+		 * so switching it here - before the child exists - is equivalent to
+		 * doing it afterwards, and keeps this failure out of the window in
+		 * which a spawned child would have to be cleaned up. */
+		if ((flags & HAWK_PIO_INNOBLOCK) && set_pipe_nonblock(pio, handle[1], 1) <= -1) goto oops;
+
 		minidx = 0; maxidx = 1;
 	}
 
@@ -750,6 +765,8 @@ int hawk_pio_init (hawk_pio_t* pio, hawk_gem_t* gem, const hawk_ooch_t* cmd, int
 			}
 		}
 
+		if ((flags & HAWK_PIO_OUTNOBLOCK) && set_pipe_nonblock(pio, handle[2], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 2;
 		maxidx = 3;
 	}
@@ -776,6 +793,8 @@ int hawk_pio_init (hawk_pio_t* pio, hawk_gem_t* gem, const hawk_ooch_t* cmd, int
 			}
 		}
 
+		if ((flags & HAWK_PIO_ERRNOBLOCK) && set_pipe_nonblock(pio, handle[4], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 4;
 		maxidx = 5;
 	}
@@ -813,7 +832,7 @@ int hawk_pio_init (hawk_pio_t* pio, hawk_gem_t* gem, const hawk_ooch_t* cmd, int
 
 	startup.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
 	startup.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
-	startup.hStdOutput = GetStdHandle(STD_ERROR_HANDLE);
+	startup.hStdError = GetStdHandle(STD_ERROR_HANDLE);
 	if (startup.hStdInput == INVALID_HANDLE_VALUE ||
 	    startup.hStdOutput == INVALID_HANDLE_VALUE ||
 	    startup.hStdError == INVALID_HANDLE_VALUE)
@@ -1013,6 +1032,12 @@ create_process:
 		DosQueryFHState (handle[1], &state);
 		DosSetFHState (handle[1], state | OPEN_FLAGS_NOINHERIT); */
 
+		/* handle[1] is the parent's end and is marked NOINHERIT above, so
+		 * switching it before the child exists is equivalent to doing it
+		 * afterwards, and keeps this failure out of the window in which a
+		 * spawned child would have to be cleaned up. */
+		if ((flags & HAWK_PIO_INNOBLOCK) && set_pipe_nonblock(pio, handle[1], 1) <= -1) goto oops;
+
 		minidx = 0; maxidx = 1;
 	}
 
@@ -1035,6 +1060,8 @@ create_process:
 			goto oops;
 		}
 
+		if ((flags & HAWK_PIO_OUTNOBLOCK) && set_pipe_nonblock(pio, handle[2], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 2;
 		maxidx = 3;
 	}
@@ -1058,6 +1085,8 @@ create_process:
 			goto oops;
 		}
 
+		if ((flags & HAWK_PIO_ERRNOBLOCK) && set_pipe_nonblock(pio, handle[4], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 4;
 		maxidx = 5;
 	}
@@ -1303,6 +1332,15 @@ create_process:
 			hawk_gem_seterrnum(pio->gem, HAWK_NULL, hawk_syserr_to_errnum(errno));
 			goto oops;
 		}
+
+		/* O_NONBLOCK belongs to the open file description, and handle[1] is
+		 * the parent's end - a different description from the handle[0] the
+		 * child inherits, and one the child closes anyway. so switching it
+		 * here, before the child exists, is equivalent to doing it after the
+		 * spawn, and keeps this failure out of the window in which a spawned
+		 * child would have to be cleaned up. */
+		if ((flags & HAWK_PIO_INNOBLOCK) && set_pipe_nonblock(pio, handle[1], 1) <= -1) goto oops;
+
 		minidx = 0; maxidx = 1;
 	}
 
@@ -1313,6 +1351,9 @@ create_process:
 			hawk_gem_seterrnum(pio->gem, HAWK_NULL, hawk_syserr_to_errnum(errno));
 			goto oops;
 		}
+
+		if ((flags & HAWK_PIO_OUTNOBLOCK) && set_pipe_nonblock(pio, handle[2], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 2;
 		maxidx = 3;
 	}
@@ -1324,6 +1365,9 @@ create_process:
 			hawk_gem_seterrnum(pio->gem, HAWK_NULL, hawk_syserr_to_errnum(errno));
 			goto oops;
 		}
+
+		if ((flags & HAWK_PIO_ERRNOBLOCK) && set_pipe_nonblock(pio, handle[4], 1) <= -1) goto oops;
+
 		if (minidx == -1) minidx = 4;
 		maxidx = 5;
 	}
@@ -1761,13 +1805,6 @@ create_process:
 	}
 #endif
 
-	if (((flags & HAWK_PIO_INNOBLOCK) && set_pipe_nonblock(pio, handle[1], 1) <= -1) ||
-	    ((flags & HAWK_PIO_OUTNOBLOCK) && set_pipe_nonblock(pio, handle[2], 1) <= -1) ||
-	    ((flags & HAWK_PIO_ERRNOBLOCK) && set_pipe_nonblock(pio, handle[4], 1) <= -1))
-	{
-		goto oops;
-	}
-
 	/* store back references */
 	pio->pin[HAWK_PIO_IN].self = pio;
 	pio->pin[HAWK_PIO_OUT].self = pio;
@@ -1778,7 +1815,6 @@ create_process:
 	pio->pin[HAWK_PIO_OUT].handle = handle[2];
 	pio->pin[HAWK_PIO_ERR].handle = handle[4];
 
-
 	if (flags & HAWK_PIO_TEXT)
 	{
 		int topt = 0;
@@ -1791,6 +1827,9 @@ create_process:
 		{
 			int r;
 
+			/* NOTE: the child process has already been spawned.
+			 * jumping to oops for failure below will trigger forced kill of
+			 * a child process at the beginning of the oops part */
 			tio[i] = hawk_tio_open(pio->gem, HAWK_SIZEOF(&pio->pin[i]), topt);
 			if (HAWK_UNLIKELY(!tio[i])) goto oops;
 
@@ -1808,6 +1847,26 @@ create_process:
 	return 0;
 
 oops:
+	/* if the child has already been spawned, a failure in a later step must
+	 * not leave it behind: the caller is about to get -1 and will never have
+	 * a handle to reap it with. */
+	if (pio->child != HAWK_PIO_PID_NIL)
+	{	
+		hawk_errnum_t err = hawk_gem_geterrnum(pio->gem);
+
+		/* SIGKILL, not a bare wait: the child may be long-running, or blocked
+		 * writing to a pipe we still hold - either would hang us here. */
+		hawk_pio_kill(pio);
+
+		/* the caller's WAITNOBLOCK/WAITNORETRY must not stop us reaping */
+		pio->flags &= ~HAWK_PIO_WAITNOBLOCK;
+		pio->flags &= ~HAWK_PIO_WAITNORETRY;
+		hawk_pio_wait(pio); /* reaps and resets child to PID_NIL */
+
+		/* kill()/wait() set errnum themselves - restore the real cause */
+		hawk_gem_seterrnum (pio->gem, HAWK_NULL, err);	
+	}
+
 #if defined(_WIN32)
 	if (windevnul != INVALID_HANDLE_VALUE) CloseHandle(windevnul);
 
@@ -1837,11 +1896,20 @@ oops:
 	}
 
 #if defined(_WIN32)
-	for (i = minidx; i < maxidx; i++) CloseHandle(handle[i]);
+	if (minidx >= 0)
-#elif defined(__OS2__)
-	for (i = minidx; i < maxidx; i++)
 	{
-		if (handle[i] != HAWK_PIO_HND_NIL) DosClose(handle[i]);
+		for (i = minidx; i <= maxidx; i++)
+		{
+			if (handle[i] != HAWK_PIO_HND_NIL) CloseHandle(handle[i]);
+		}
+	}
+#elif defined(__OS2__)
+	if (minidx >= 0)
+	{
+		for (i = minidx; i <= maxidx; i++)
+		{
+			if (handle[i] != HAWK_PIO_HND_NIL) DosClose(handle[i]);
+		}
 	}
 #elif defined(__DOS__)
 
@@ -1852,19 +1920,28 @@ oops:
 		posix_spawn_file_actions_destroy (&fa);
 		fa_inited = 0;
 	}
-	for (i = minidx; i < maxidx; i++)
+	if (i >= 0)
 	{
-		if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		for (i = minidx; i <= maxidx; i++)
+		{
+			if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		}
 	}
 #elif defined(HAWK_SYSCALL0) && defined(SYS_vfork)
-	for (i = minidx; i < maxidx; i++)
+	if (i >= 0)
 	{
-		if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		for (i = minidx; i <= maxidx; i++)
+		{
+			if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		}
 	}
 #else
-	for (i = minidx; i < maxidx; i++)
+	if (i >= 0)
 	{
-		if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		for (i = minidx; i <= maxidx; i++)
+		{
+			if (handle[i] != HAWK_PIO_HND_NIL) HAWK_CLOSE(handle[i]);
+		}
 	}
 #endif