Fix race condition in win32 signal handling.

There was a race condition where the receiving pipe could be closed by the
child thread if the main thread was pre-empted before it got a chance to
create a new one, and the dispatch thread ran to completion during that time.

One symptom of this is that rows in pg_listener could be dropped under
heavy load.

Analysis and original patch by Radu Ilie, with some small
modifications by Magnus Hagander.

1 files changed, 43 insertions, 5 deletions

diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index eb95984c7b3..1ffc55a5c4d 100644
--- a/src/backend/port/win32/signal.c
+++ b/src/backend/port/win32/signal.c
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/port/win32/signal.c,v 1.23 2010/01/02 16:57:50 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/port/win32/signal.c,v 1.24 2010/01/31 17:16:23 mha Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -275,6 +275,33 @@ 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.
+			 */
+			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)
+			{
+				/*
+				 * 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.
+				 */
+				write_stderr("could not create signal listener pipe: error code %d; retrying\n", (int) GetLastError());
+				/*
+				 * Keep going so we at least dispatch this signal. Hopefully, the call will succeed
+				 * when retried in the loop soon after.
+				 */
+			}
 			hThread = CreateThread(NULL, 0,
 						  (LPTHREAD_START_ROUTINE) pg_signal_dispatch_thread,
 								   (LPVOID) pipe, 0, NULL);
@@ -283,13 +310,24 @@ pg_signal_thread(LPVOID param)
 							 (int) GetLastError());
 			else
 				CloseHandle(hThread);
+
+			/*
+			 * 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;
 		}
 		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.
+			 */
 			CloseHandle(pipe);
-
-		/* Set up so we create a new pipe on next loop */
-		pipe = INVALID_HANDLE_VALUE;
+			pipe = INVALID_HANDLE_VALUE;
+		}
 	}
 	return 0;
 }