Use only one thread to handle incoming signals on Windows.

tglsfdc · tglsfdc · commit 16114f2ea0c0 · 2019-12-11T15:09:54.000-05:00
Since its inception, our Windows signal emulation code has worked by running a main signal thread that just watches for incoming signal requests, and then spawns a new thread to handle each such request. That design is meant for servers in which requests can take substantial effort to process, and it's worth parallelizing the handling of requests. But those assumptions are just bogus for our signal code. It's not much more than pg_queue_signal(), which is cheap and can't parallelize at all, plus we don't really expect lots of signals to arrive at the same backend at once. More importantly, this approach creates failure modes that we could do without: either inability to spawn a new thread or inability to create a new pipe handle will risk loss of signals. Hence, dispense with the separate per-signal threads and just service each request in-line in the main signal thread. This should be a bit faster (for the normal case of one signal at a time) as well as more robust. Patch by me; thanks to Andrew Dunstan for testing and Amit Kapila for review. Discussion: https://postgr.es/m/4412.1575748586@sss.pgh.pa.us
diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
@@ -225,77 +225,19 @@ pg_queue_signal(int signum)
 	SetEvent(pgwin32_signal_event);
 }
 
-/*
- * Signal dispatching thread.  This runs after we have received a named
- * pipe connection from a client (signal sender).   Process the request,
- * close the pipe, and exit.
- */
-static DWORD WINAPI
-pg_signal_dispatch_thread(LPVOID param)
-{
-	HANDLE		pipe = (HANDLE) param;
-	BYTE		sigNum;
-	DWORD		bytes;
-
-	if (!ReadFile(pipe, &sigNum, 1, &bytes, NULL))
-	{
-		/* Client died before sending */
-		CloseHandle(pipe);
-		return 0;
-	}
-	if (bytes != 1)
-	{
-		/* Received <bytes> bytes over signal pipe (should be 1) */
-		CloseHandle(pipe);
-		return 0;
-	}
-
-	/*
-	 * Queue the signal before responding to the client.  In this way, it's
-	 * guaranteed that once kill() has returned in the signal sender, the next
-	 * CHECK_FOR_INTERRUPTS() in the signal recipient will see the signal.
-	 * (This is a stronger guarantee than POSIX makes; maybe we don't need it?
-	 * But without it, we've seen timing bugs on Windows that do not manifest
-	 * on any known Unix.)
-	 */
-	pg_queue_signal(sigNum);
-
-	/*
-	 * Write something back to the client, allowing its CallNamedPipe() call
-	 * to terminate.
-	 */
-	WriteFile(pipe, &sigNum, 1, &bytes, NULL);	/* Don't care if it works or
-												 * not.. */
-
-	/*
-	 * We must wait for the client to read the data before we can close the
-	 * pipe, else the data will be lost.  (If the WriteFile call failed,
-	 * there'll be nothing in the buffer, so this shouldn't block.)
-	 */
-	FlushFileBuffers(pipe);
-
-	/* This is a formality, since we're about to close the pipe anyway. */
-	DisconnectNamedPipe(pipe);
-
-	/* And we're done. */
-	CloseHandle(pipe);
-
-	return 0;
-}
-
 /* Signal handling thread */
 static DWORD WINAPI
 pg_signal_thread(LPVOID param)
 {
 	char		pipename[128];
 	HANDLE		pipe = pgwin32_initial_signal_pipe;
 
+	/* Set up pipe name, in case we have to re-create the pipe. */
 	snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%lu", GetCurrentProcessId());
 
 	for (;;)
 	{
 		BOOL		fConnected;
-		HANDLE		hThread;
 
 		/* Create a new pipe instance if we don't have one. */
 		if (pipe == INVALID_HANDLE_VALUE)
@@ -320,59 +262,62 @@ pg_signal_thread(LPVOID param)
 		fConnected = ConnectNamedPipe(pipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
 		if (fConnected)
 		{
-			HANDLE		newpipe;
-
 			/*
-			 * We have a connected pipe. Pass this off to a separate thread
-			 * that will do the actual processing of the pipe.
-			 *
-			 * We must also create a new instance of the pipe *before* we
-			 * start running the new thread. If we don't, there is a race
-			 * condition whereby the dispatch thread might run CloseHandle()
-			 * before we have created a new instance, thereby causing a small
-			 * window of time where we will miss incoming requests.
+			 * We have a connection from a would-be signal sender. Process it.
 			 */
-			newpipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
-									  PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
-									  PIPE_UNLIMITED_INSTANCES, 16, 16, 1000, NULL);
-			if (newpipe == INVALID_HANDLE_VALUE)
+			BYTE		sigNum;
+			DWORD		bytes;
+
+			if (ReadFile(pipe, &sigNum, 1, &bytes, NULL) &&
+				bytes == 1)
 			{
 				/*
-				 * This really should never fail. Just retry in case it does,
-				 * even though we have a small race window in that case. There
-				 * is nothing else we can do other than abort the whole
-				 * process which will be even worse.
+				 * Queue the signal before responding to the client.  In this
+				 * way, it's guaranteed that once kill() has returned in the
+				 * signal sender, the next CHECK_FOR_INTERRUPTS() in the
+				 * signal recipient will see the signal.  (This is a stronger
+				 * guarantee than POSIX makes; maybe we don't need it?  But
+				 * without it, we've seen timing bugs on Windows that do not
+				 * manifest on any known Unix.)
 				 */
-				write_stderr("could not create signal listener pipe: error code %lu; retrying\n", GetLastError());
+				pg_queue_signal(sigNum);
 
 				/*
-				 * Keep going so we at least dispatch this signal. Hopefully,
-				 * the call will succeed when retried in the loop soon after.
+				 * Write something back to the client, allowing its
+				 * CallNamedPipe() call to terminate.
 				 */
+				WriteFile(pipe, &sigNum, 1, &bytes, NULL);	/* Don't care if it
+															 * works or not */
+
+				/*
+				 * We must wait for the client to read the data before we can
+				 * disconnect, else the data will be lost.  (If the WriteFile
+				 * call failed, there'll be nothing in the buffer, so this
+				 * shouldn't block.)
+				 */
+				FlushFileBuffers(pipe);
 			}
-			hThread = CreateThread(NULL, 0,
-								   (LPTHREAD_START_ROUTINE) pg_signal_dispatch_thread,
-								   (LPVOID) pipe, 0, NULL);
-			if (hThread == INVALID_HANDLE_VALUE)
-				write_stderr("could not create signal dispatch thread: error code %lu\n",
-							 GetLastError());
 			else
-				CloseHandle(hThread);
+			{
+				/*
+				 * If we fail to read a byte from the client, assume it's the
+				 * client's problem and do nothing.  Perhaps it'd be better to
+				 * force a pipe close and reopen?
+				 */
+			}
 
-			/*
-			 * Background thread is running with our instance of the pipe. So
-			 * replace our reference with the newly created one and loop back
-			 * up for another run.
-			 */
-			pipe = newpipe;
+			/* Disconnect from client so that we can re-use the pipe. */
+			DisconnectNamedPipe(pipe);
 		}
 		else
 		{
 			/*
-			 * Connection failed. Cleanup and try again.
+			 * Connection failed.  Cleanup and try again.
 			 *
-			 * This should never happen. If it does, we have a small race
-			 * condition until we loop up and re-create the pipe.
+			 * This should never happen.  If it does, there's a window where
+			 * we'll miss signals until we manage to re-create the pipe.
+			 * However, just trying to use the same pipe again is probably not
+			 * going to work, so we have little choice.
 			 */
 			CloseHandle(pipe);
 			pipe = INVALID_HANDLE_VALUE;
