Fix lost Windows socket EOF events.

macdice · macdice · commit 3554d841d216 · 2024-07-13T15:44:11.000+12:00
Winsock only signals an FD_CLOSE event once if the other end of the socket shuts down gracefully. Because each WaitLatchOrSocket() call constructs and destroys a new event handle every time, with unlucky timing we can lose it and hang. We get away with this only if the other end disconnects non-gracefully, because FD_CLOSE is repeatedly signaled in that case. To fix this design flaw in our Windows socket support fundamentally, we'd probably need to rearchitect it so that a single event handle exists for the lifetime of a socket, or switch to completely different multiplexing or async I/O APIs. That's going to be a bigger job and probably wouldn't be back-patchable. This brute force kludge closes the race by explicitly polling with MSG_PEEK before sleeping. Back-patch to all supported releases. This should hopefully clear up some random build farm and CI hang failures reported over the years. It might also allow us to try using graceful shutdown in more places again (reverted in commit 29992a6) to fix instability in the transmission of FATAL error messages, but that isn't done by this commit. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Tested-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/176008.1715492071%40sss.pgh.pa.us
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
@@ -1709,6 +1709,38 @@ WaitEventSetWaitBlock(WaitEventSet *set, int cur_timeout,
 			cur_event->reset = false;
 		}
 
+		/*
+		 * We associate the socket with a new event handle for each
+		 * WaitEventSet.  FD_CLOSE is only generated once if the other end
+		 * closes gracefully.  Therefore we might miss the FD_CLOSE
+		 * notification, if it was delivered to another event after we stopped
+		 * waiting for it.  Close that race by peeking for EOF after setting
+		 * up this handle to receive notifications, and before entering the
+		 * sleep.
+		 *
+		 * XXX If we had one event handle for the lifetime of a socket, we
+		 * wouldn't need this.
+		 */
+		if (cur_event->events & WL_SOCKET_READABLE)
+		{
+			char		c;
+			WSABUF		buf;
+			DWORD		received;
+			DWORD		flags;
+
+			buf.buf = &c;
+			buf.len = 1;
+			flags = MSG_PEEK;
+			if (WSARecv(cur_event->fd, &buf, 1, &received, &flags, NULL, NULL) == 0)
+			{
+				occurred_events->pos = cur_event->pos;
+				occurred_events->user_data = cur_event->user_data;
+				occurred_events->events = WL_SOCKET_READABLE;
+				occurred_events->fd = cur_event->fd;
+				return 1;
+			}
+		}
+
 		/*
 		 * Windows does not guarantee to log an FD_WRITE network event
 		 * indicating that more data can be sent unless the previous send()
