From: Tom Lane Date: Sun, 13 May 2012 18:35:40 +0000 (-0400) Subject: Attempt to fix some issues in our Windows socket code. X-Git-Tag: REL9_2_BETA2~92 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b85427f2276d02756b558c0024949305ea65aca5;p=postgresql Attempt to fix some issues in our Windows socket code. Make sure WaitLatchOrSocket regards FD_CLOSE as a read-ready condition. We might want to tweak this further, but it was surely wrong as-is. Make pgwin32_waitforsinglesocket detach its private event object from the passed socket before returning. I suspect that failure to do so leads to race conditions when other code (such as WaitLatchOrSocket) attaches a different event object to the same socket. Moreover, the existing coding meant that repeated calls to pgwin32_waitforsinglesocket would perform ResetEvent on an event actively connected to a socket, which is rumored to be an unsafe practice; the WSAEventSelect documentation appears to recommend against this, though it does not say not to do it in so many words. Also, uniformly use the coding pattern "WSAEventSelect(s, NULL, 0)" to detach events from sockets, rather than passing the event in the second parameter. The WSAEventSelect documentation says that the second parameter is ignored if the third is 0, so theoretically this should make no difference. However, elsewhere on the same reference page the use of NULL in this context is recommended, and I have found suggestions on the net that some versions of Windows have bugs with a non-NULL second parameter in this usage. Some other mostly-cosmetic cleanup, such as using the right one of WSAGetLastError and GetLastError for reporting errors from these functions. --- diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c index b2c6325328..a7215cad6e 100644 --- a/src/backend/port/win32/socket.c +++ b/src/backend/port/win32/socket.c @@ -137,6 +137,7 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) HANDLE events[2]; int r; + /* Create an event object just once and use it on all future calls */ if (waitevent == INVALID_HANDLE_VALUE) { waitevent = CreateEvent(NULL, TRUE, FALSE, NULL); @@ -150,20 +151,19 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) (errmsg_internal("could not reset socket waiting event: error code %lu", GetLastError()))); /* - * make sure we don't multiplex this kernel event object with a different - * socket from a previous call + * Track whether socket is UDP or not. (NB: most likely, this is both + * useless and wrong; there is no reason to think that the behavior of + * WSAEventSelect is different for TCP and UDP.) */ - if (current_socket != s) - { - if (current_socket != -1) - WSAEventSelect(current_socket, waitevent, 0); isUDP = isDataGram(s); - } - current_socket = s; - if (WSAEventSelect(s, waitevent, what) == SOCKET_ERROR) + /* + * Attach event to socket. NOTE: we must detach it again before returning, + * since other bits of code may try to attach other events to the socket. + */ + if (WSAEventSelect(s, waitevent, what) != 0) { TranslateSocketError(); return 0; @@ -196,10 +196,14 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) r = WSASend(s, &buf, 1, &sent, 0, NULL, NULL); if (r == 0) /* Completed - means things are fine! */ + { + WSAEventSelect(s, NULL, 0); return 1; + } else if (WSAGetLastError() != WSAEWOULDBLOCK) { TranslateSocketError(); + WSAEventSelect(s, NULL, 0); return 0; } } @@ -210,6 +214,8 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) else r = WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE); + WSAEventSelect(s, NULL, 0); + if (r == WAIT_OBJECT_0 || r == WAIT_IO_COMPLETION) { pgwin32_dispatch_queued_signals(); @@ -219,9 +225,12 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout) if (r == WAIT_OBJECT_0 + 1) return 1; if (r == WAIT_TIMEOUT) + { + errno = EWOULDBLOCK; return 0; + } ereport(ERROR, - (errmsg_internal("unrecognized return value from WaitForMultipleObjects: %d (%lu)", r, GetLastError()))); + (errmsg_internal("unrecognized return value from WaitForMultipleObjects: %d (error code %lu)", r, GetLastError()))); return 0; } @@ -543,9 +552,12 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c if (writefds && FD_ISSET(sockets[i], writefds)) flags |= FD_WRITE | FD_CLOSE; - if (WSAEventSelect(sockets[i], events[i], flags) == SOCKET_ERROR) + if (WSAEventSelect(sockets[i], events[i], flags) != 0) { TranslateSocketError(); + /* release already-assigned event objects */ + while (--i >= 0) + WSAEventSelect(sockets[i], NULL, 0); for (i = 0; i < numevents; i++) WSACloseEvent(events[i]); return -1; @@ -565,9 +577,9 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c for (i = 0; i < numevents; i++) { ZeroMemory(&resEvents, sizeof(resEvents)); - if (WSAEnumNetworkEvents(sockets[i], events[i], &resEvents) == SOCKET_ERROR) - ereport(FATAL, - (errmsg_internal("failed to enumerate network events: error code %lu", GetLastError()))); + if (WSAEnumNetworkEvents(sockets[i], events[i], &resEvents) != 0) + elog(ERROR, "failed to enumerate network events: error code %u", + WSAGetLastError()); /* Read activity? */ if (readfds && FD_ISSET(sockets[i], readfds)) { @@ -594,10 +606,10 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c } } - /* Clean up all handles */ + /* Clean up all the event objects */ for (i = 0; i < numevents; i++) { - WSAEventSelect(sockets[i], events[i], 0); + WSAEventSelect(sockets[i], NULL, 0); WSACloseEvent(events[i]); } diff --git a/src/backend/port/win32_latch.c b/src/backend/port/win32_latch.c index 622798924d..62800ad66f 100644 --- a/src/backend/port/win32_latch.c +++ b/src/backend/port/win32_latch.c @@ -130,10 +130,11 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock, numevents = 2; if (wakeEvents & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE)) { + /* Need an event object to represent events on the socket */ int flags = 0; if (wakeEvents & WL_SOCKET_READABLE) - flags |= FD_READ; + flags |= (FD_READ | FD_CLOSE); if (wakeEvents & WL_SOCKET_WRITEABLE) flags |= FD_WRITE; @@ -201,11 +202,11 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock, WSANETWORKEVENTS resEvents; ZeroMemory(&resEvents, sizeof(resEvents)); - if (WSAEnumNetworkEvents(sock, sockevent, &resEvents) == SOCKET_ERROR) - elog(ERROR, "failed to enumerate network events: error code %lu", - GetLastError()); + if (WSAEnumNetworkEvents(sock, sockevent, &resEvents) != 0) + elog(ERROR, "failed to enumerate network events: error code %u", + WSAGetLastError()); if ((wakeEvents & WL_SOCKET_READABLE) && - (resEvents.lNetworkEvents & FD_READ)) + (resEvents.lNetworkEvents & (FD_READ | FD_CLOSE))) { result |= WL_SOCKET_READABLE; } @@ -233,10 +234,10 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock, } while (result == 0); - /* Clean up the handle we created for the socket */ + /* Clean up the event object we created for the socket */ if (sockevent != WSA_INVALID_EVENT) { - WSAEventSelect(sock, sockevent, 0); + WSAEventSelect(sock, NULL, 0); WSACloseEvent(sockevent); }