From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
cc: Amit Kapila <amit.kapila16@gmail.com>,
        Magnus Hagander <magnus@hagander.net>,
        Mark Dilger <hornschnorter@gmail.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Windows buildfarm members vs. new async-notify isolation test
In-reply-to: <28011.1575816794@sss.pgh.pa.us>
References: <32745.1575303812@sss.pgh.pa.us>
 <7221b3b4-fc7f-404d-5cf6-830d3aa6800d@2ndQuadrant.com>
 <362bca0b-1c1c-c760-ab19-b5d9a14c69ea@gmail.com>
 <13003.1575391248@sss.pgh.pa.us>
 <CAA4eK1+qevZ=sB1nW05_gkiHLWjUU0psNPiutFnQi=+aCztLXA@mail.gmail.com>
 <25746.1575436347@sss.pgh.pa.us>
 <CAA8=A78X-0vVd5tk7r=Z7yEWW5Vy1=46q7wjZaOa8Xaed5FooQ@mail.gmail.com>
 <CAA4eK1+HOFzs=-MHDz6ef936KuuHJBxZbHEi7xt9TBqgHNO9GQ@mail.gmail.com>
 <e1eb7182-a487-42f1-e113-4e5758e8ceef@2ndQuadrant.com>
 <20102.1575675105@sss.pgh.pa.us>
 <CAA4eK1LRwxr51JuXsgpmCA+-uswCHiGvhZ88jPz8VeJxfyv65g@mail.gmail.com>
 <30330.1575739251@sss.pgh.pa.us> <4412.1575748586@sss.pgh.pa.us>
 <CAA4eK1KpRMRJG0krbiL8sUA9wZTVwvoHejEkJK2sVH2idG-rSQ@mail.gmail.com>
 <CAA4eK1+eo0t20xkPX9-WGdO+tDwEv9krba-R2F3OSqyLAWW59w@mail.gmail.com>
 <28011.1575816794@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sun, 08 Dec 2019 09:53:14 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <7581.1575822038.0@sss.pgh.pa.us>
Date: Sun, 08 Dec 2019 11:22:08 -0500
Message-ID: <7634.1575822128@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7581.1575822038.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="this-might-fix-windows-race-condition.patch";
	charset="us-ascii"
Content-ID: <7581.1575822038.2@sss.pgh.pa.us>
Content-Description: this-might-fix-windows-race-condition.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/sign=
al.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 !=3D NULL);
 	if (signum >=3D PG_SIGNAL_COUNT || signum <=3D 0)
-		return;
+		return;					/* ignore any bad signal number */
 =

 	EnterCriticalSection(&pg_signal_crit_sec);
 	pg_signal_queue |=3D 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 ne=
xt
+	 * CHECK_FOR_INTERRUPTS() in the signal recipient will see the signal.
+	 * (This is a stronger guarantee than POSIX makes; maybe we don't need i=
t?
+	 * But without it, we've seen timing bugs on Windows that do not manifes=
t
+	 * 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 =3D=3D INVALID_HANDLE_VALUE)
 		{
 			pipe =3D 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 =3D ConnectNamedPipe(pipe, NULL) ? TRUE : (GetLastError() =3D=
=3D ERROR_PIPE_CONNECTED);
 		if (fConnected)
 		{

------- =_aaaaaaaaaa0--