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

I wrote:
> So, just idly looking at the code in src/backend/port/win32/signal.c
> and src/port/kill.c, I have to wonder why we have this baroque-looking
> design of using *two* signal management threads.  And, if I'm
> reading it right, we create an entire new pipe object and an entire
> new instance of the second thread for each incoming signal.  Plus, the
> signal senders use CallNamedPipe (hence, underneath, TransactNamedPipe)
> which means they in effect wait for the recipient's signal-handling
> thread to ack receipt of the signal.  Maybe there's a good reason for
> all this but it sure seems like a lot of wasted cycles from here.

Here's a possible patch (untested by me) to get rid of the second thread
and the new-pipe-for-every-request behavior.  I believe that the existing
logic may be based on Microsoft's "Multithreaded Pipe Server" example [1]
or something similar, but that's based on an assumption that servicing
a client request may take a substantial amount of time and it's worth
handling requests concurrently.  Neither point applies in this context.

Doing it like this seems attractive to me because it gets rid of two
different failure modes: inability to create a new thread and inability
to create a new pipe handle.  Now on the other hand, it means that
inability to complete the read/write transaction with a client right
away will delay processing of other signals.  But we know that the
client is engaged in a CallNamedPipe operation, so how realistic is
that concern?

This is to be applied on top of the other patch I just sent.

            regards, tom lane

[1] https://docs.microsoft.com/en-us/windows/win32/ipc/multithreaded-pipe-server

diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.c
index f1e35fd..aa762b9 100644
--- a/src/backend/port/win32/signal.c
+++ b/src/backend/port/win32/signal.c
@@ -38,7 +38,7 @@ static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
 static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];


-/* Signal handling thread function */
+/* Signal handling thread functions */
 static DWORD WINAPI pg_signal_thread(LPVOID param);
 static BOOL WINAPI pg_console_handler(DWORD dwCtrlType);

@@ -225,64 +225,6 @@ 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)
@@ -290,12 +232,12 @@ 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,51 +262,52 @@ 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
         {