Re: Windows buildfarm members vs. new async-notify isolation test

I wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> IIUC, once the dispatch thread has queued the signal
>> (pg_queue_signal), the next CHECK_FOR_INTERRUPTS by the main thread
>> will execute the signal.  So, if we move pg_queue_signal() before we
>> do WriteFile in pg_signal_dispatch_thread(), this race condition will
>> be closed.  Now, we might not want to do this as that will add some
>> more time (even though very less) before notify on the other side can
>> finish or maybe there is some technical problem with this idea which I
>> am not able to see immediately.

> Hmm.  Certainly worth trying to see if it resolves the failure on
> Andrew's machines.

For Andrew's convenience, here's a draft patch for that.  I took the
liberty of improving the rather thin comments in this area, too.

            regards, tom lane

diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index 9b5e544..f1e35fd 100644
--- a/src/backend/port/win32/signal.c
+++ b/src/backend/port/win32/signal.c
@@ -208,12 +208,15 @@ pgwin32_create_signal_listener(pid_t pid)
  */


+/*
+ * Queue a signal for the main thread, by setting the flag bit and event.
+ */
 void
 pg_queue_signal(int signum)
 {
     Assert(pgwin32_signal_event != NULL);
     if (signum >= PG_SIGNAL_COUNT || signum <= 0)
-        return;
+        return;                    /* ignore any bad signal number */

     EnterCriticalSection(&pg_signal_crit_sec);
     pg_signal_queue |= sigmask(signum);
@@ -222,7 +225,11 @@ pg_queue_signal(int signum)
     SetEvent(pgwin32_signal_event);
 }

-/* Signal dispatching thread */
+/*
+ * 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)
 {
@@ -242,13 +249,37 @@ pg_signal_dispatch_thread(LPVOID param)
         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);

-    pg_queue_signal(sigNum);
     return 0;
 }

@@ -266,6 +297,7 @@ pg_signal_thread(LPVOID param)
         BOOL        fConnected;
         HANDLE        hThread;

+        /* Create a new pipe instance if we don't have one. */
         if (pipe == INVALID_HANDLE_VALUE)
         {
             pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
@@ -280,6 +312,11 @@ pg_signal_thread(LPVOID param)
             }
         }

+        /*
+         * Wait for a client to connect.  If something connects before we
+         * reach here, we'll get back a "failure" with ERROR_PIPE_CONNECTED,
+         * which is actually a success (way to go, Microsoft).
+         */
         fConnected = ConnectNamedPipe(pipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
         if (fConnected)
         {