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.

diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index eb95984c7b32cca30c6cfcd64ff7be33697213d6..1ffc55a5c4dbb733b45c37eeefaeb4f239b9c27f 100644 (file)
--- 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;
 }