Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit b85427f

Browse files
committed
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.
1 parent fd350ef commit b85427f

File tree

2 files changed

+36
-23
lines changed

2 files changed

+36
-23
lines changed

src/backend/port/win32/socket.c

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
137137
HANDLE events[2];
138138
int r;
139139

140+
/* Create an event object just once and use it on all future calls */
140141
if (waitevent == INVALID_HANDLE_VALUE)
141142
{
142143
waitevent = CreateEvent(NULL, TRUE, FALSE, NULL);
@@ -150,20 +151,19 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
150151
(errmsg_internal("could not reset socket waiting event: error code %lu", GetLastError())));
151152

152153
/*
153-
* make sure we don't multiplex this kernel event object with a different
154-
* socket from a previous call
154+
* Track whether socket is UDP or not. (NB: most likely, this is both
155+
* useless and wrong; there is no reason to think that the behavior of
156+
* WSAEventSelect is different for TCP and UDP.)
155157
*/
156-
157158
if (current_socket != s)
158-
{
159-
if (current_socket != -1)
160-
WSAEventSelect(current_socket, waitevent, 0);
161159
isUDP = isDataGram(s);
162-
}
163-
164160
current_socket = s;
165161

166-
if (WSAEventSelect(s, waitevent, what) == SOCKET_ERROR)
162+
/*
163+
* Attach event to socket. NOTE: we must detach it again before returning,
164+
* since other bits of code may try to attach other events to the socket.
165+
*/
166+
if (WSAEventSelect(s, waitevent, what) != 0)
167167
{
168168
TranslateSocketError();
169169
return 0;
@@ -196,10 +196,14 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
196196

197197
r = WSASend(s, &buf, 1, &sent, 0, NULL, NULL);
198198
if (r == 0) /* Completed - means things are fine! */
199+
{
200+
WSAEventSelect(s, NULL, 0);
199201
return 1;
202+
}
200203
else if (WSAGetLastError() != WSAEWOULDBLOCK)
201204
{
202205
TranslateSocketError();
206+
WSAEventSelect(s, NULL, 0);
203207
return 0;
204208
}
205209
}
@@ -210,6 +214,8 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
210214
else
211215
r = WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE);
212216

217+
WSAEventSelect(s, NULL, 0);
218+
213219
if (r == WAIT_OBJECT_0 || r == WAIT_IO_COMPLETION)
214220
{
215221
pgwin32_dispatch_queued_signals();
@@ -219,9 +225,12 @@ pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
219225
if (r == WAIT_OBJECT_0 + 1)
220226
return 1;
221227
if (r == WAIT_TIMEOUT)
228+
{
229+
errno = EWOULDBLOCK;
222230
return 0;
231+
}
223232
ereport(ERROR,
224-
(errmsg_internal("unrecognized return value from WaitForMultipleObjects: %d (%lu)", r, GetLastError())));
233+
(errmsg_internal("unrecognized return value from WaitForMultipleObjects: %d (error code %lu)", r, GetLastError())));
225234
return 0;
226235
}
227236

@@ -543,9 +552,12 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c
543552
if (writefds && FD_ISSET(sockets[i], writefds))
544553
flags |= FD_WRITE | FD_CLOSE;
545554

546-
if (WSAEventSelect(sockets[i], events[i], flags) == SOCKET_ERROR)
555+
if (WSAEventSelect(sockets[i], events[i], flags) != 0)
547556
{
548557
TranslateSocketError();
558+
/* release already-assigned event objects */
559+
while (--i >= 0)
560+
WSAEventSelect(sockets[i], NULL, 0);
549561
for (i = 0; i < numevents; i++)
550562
WSACloseEvent(events[i]);
551563
return -1;
@@ -565,9 +577,9 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c
565577
for (i = 0; i < numevents; i++)
566578
{
567579
ZeroMemory(&resEvents, sizeof(resEvents));
568-
if (WSAEnumNetworkEvents(sockets[i], events[i], &resEvents) == SOCKET_ERROR)
569-
ereport(FATAL,
570-
(errmsg_internal("failed to enumerate network events: error code %lu", GetLastError())));
580+
if (WSAEnumNetworkEvents(sockets[i], events[i], &resEvents) != 0)
581+
elog(ERROR, "failed to enumerate network events: error code %u",
582+
WSAGetLastError());
571583
/* Read activity? */
572584
if (readfds && FD_ISSET(sockets[i], readfds))
573585
{
@@ -594,10 +606,10 @@ pgwin32_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, c
594606
}
595607
}
596608

597-
/* Clean up all handles */
609+
/* Clean up all the event objects */
598610
for (i = 0; i < numevents; i++)
599611
{
600-
WSAEventSelect(sockets[i], events[i], 0);
612+
WSAEventSelect(sockets[i], NULL, 0);
601613
WSACloseEvent(events[i]);
602614
}
603615

src/backend/port/win32_latch.c

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,11 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
130130
numevents = 2;
131131
if (wakeEvents & (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE))
132132
{
133+
/* Need an event object to represent events on the socket */
133134
int flags = 0;
134135

135136
if (wakeEvents & WL_SOCKET_READABLE)
136-
flags |= FD_READ;
137+
flags |= (FD_READ | FD_CLOSE);
137138
if (wakeEvents & WL_SOCKET_WRITEABLE)
138139
flags |= FD_WRITE;
139140

@@ -201,11 +202,11 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
201202
WSANETWORKEVENTS resEvents;
202203

203204
ZeroMemory(&resEvents, sizeof(resEvents));
204-
if (WSAEnumNetworkEvents(sock, sockevent, &resEvents) == SOCKET_ERROR)
205-
elog(ERROR, "failed to enumerate network events: error code %lu",
206-
GetLastError());
205+
if (WSAEnumNetworkEvents(sock, sockevent, &resEvents) != 0)
206+
elog(ERROR, "failed to enumerate network events: error code %u",
207+
WSAGetLastError());
207208
if ((wakeEvents & WL_SOCKET_READABLE) &&
208-
(resEvents.lNetworkEvents & FD_READ))
209+
(resEvents.lNetworkEvents & (FD_READ | FD_CLOSE)))
209210
{
210211
result |= WL_SOCKET_READABLE;
211212
}
@@ -233,10 +234,10 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
233234
}
234235
while (result == 0);
235236

236-
/* Clean up the handle we created for the socket */
237+
/* Clean up the event object we created for the socket */
237238
if (sockevent != WSA_INVALID_EVENT)
238239
{
239-
WSAEventSelect(sock, sockevent, 0);
240+
WSAEventSelect(sock, NULL, 0);
240241
WSACloseEvent(sockevent);
241242
}
242243

0 commit comments

Comments
 (0)