A few win32 fixes.
-) Better handling of orphaned sockets by tracking the pending operations in it, instead of the layer above.
-) Ignoring after-shutdown operations.
diff --git a/src/core/iomgr/socket_windows.c b/src/core/iomgr/socket_windows.c
index 9306310..35dbfa1 100644
--- a/src/core/iomgr/socket_windows.c
+++ b/src/core/iomgr/socket_windows.c
@@ -75,15 +75,14 @@
/* Abandons a socket. Either we're going to queue it up for garbage collecting
from the IO Completion Port thread, or destroy it immediately. Note that this
mechanisms assumes that we're either always waiting for an operation, or we
- explicitely know that we don't. If there is a future case where we can have
+ explicitly know that we don't. If there is a future case where we can have
an "idle" socket which is neither trying to read or write, we'd start leaking
both memory and sockets. */
void grpc_winsocket_orphan(grpc_winsocket *winsocket) {
SOCKET socket = winsocket->socket;
- if (!winsocket->closed_early) {
+ if (winsocket->read_info.outstanding || winsocket->write_info.outstanding) {
grpc_iocp_socket_orphan(winsocket);
- }
- if (winsocket->closed_early) {
+ } else {
grpc_winsocket_destroy(winsocket);
}
closesocket(socket);
diff --git a/src/core/iomgr/socket_windows.h b/src/core/iomgr/socket_windows.h
index 6e778a7..8898def 100644
--- a/src/core/iomgr/socket_windows.h
+++ b/src/core/iomgr/socket_windows.h
@@ -65,12 +65,14 @@
/* The results of the overlapped operation. */
DWORD bytes_transfered;
int wsa_error;
+ /* A boolean indicating that we started an operation. */
+ int outstanding;
} grpc_winsocket_callback_info;
/* This is a wrapper to a Windows socket. A socket can have one outstanding
read, and one outstanding write. Doing an asynchronous accept means waiting
for a read operation. Doing an asynchronous connect means waiting for a
- write operation. These are completely abitrary ties between the operation
+ write operation. These are completely arbitrary ties between the operation
and the kind of event, because we can have one overlapped per pending
operation, whichever its nature is. So we could have more dedicated pending
operation callbacks for connect and listen. But given the scope of listen
@@ -87,17 +89,10 @@
/* You can't add the same socket twice to the same IO Completion Port.
This prevents that. */
int added_to_iocp;
- /* A boolean to indicate that the caller has abandonned that socket, but
+ /* A boolean to indicate that the caller has abandoned that socket, but
there is a pending operation that the IO Completion Port will have to
wait for. The socket will be collected at that time. */
int orphan;
- /* A boolean to indicate that the socket was already closed somehow, and
- that no operation is going to be pending. Trying to abandon a socket in
- that state won't result in an orphan, but will instead be destroyed
- without further delay. We could avoid that boolean by adding one into
- grpc_winsocket_callback_info describing that the operation is pending,
- but that 1) waste memory more and 2) obfuscate the intent a bit more. */
- int closed_early;
} grpc_winsocket;
/* Create a wrapped windows handle. This takes ownership of it, meaning that
diff --git a/src/core/iomgr/tcp_client_windows.c b/src/core/iomgr/tcp_client_windows.c
index 653c0c6..3e097a7 100644
--- a/src/core/iomgr/tcp_client_windows.c
+++ b/src/core/iomgr/tcp_client_windows.c
@@ -106,10 +106,8 @@
char *utf8_message = gpr_format_message(WSAGetLastError());
gpr_log(GPR_ERROR, "on_connect error: %s", utf8_message);
gpr_free(utf8_message);
- goto finish;
- } else {
+ } else if (!aborted) {
ep = grpc_tcp_create(ac->socket);
- goto finish;
}
} else {
gpr_log(GPR_ERROR, "on_connect is shutting down");
@@ -125,20 +123,12 @@
return;
}
- abort();
+ ac->socket->write_info.outstanding = 0;
-finish:
/* If we don't have an endpoint, it means the connection failed,
so it doesn't matter if it aborted or failed. We need to orphan
that socket. */
- if (!ep || aborted) {
- /* If the connection failed, it means we won't get an IOCP notification,
- so let's flag it as already closed. But if the connection was aborted,
- while we still got an endpoint, we have to wait for the IOCP to collect
- that socket. So let's properly flag that. */
- ac->socket->closed_early = !ep;
- grpc_winsocket_orphan(ac->socket);
- }
+ if (!ep || aborted) grpc_winsocket_orphan(ac->socket);
async_connect_cleanup(ac);
/* If the connection was aborted, the callback was already called when
the deadline was met. */
@@ -189,7 +179,7 @@
&ioctl_num_bytes, NULL, NULL);
if (status != 0) {
- message = "Unable to retreive ConnectEx pointer: %s";
+ message = "Unable to retrieve ConnectEx pointer: %s";
goto failure;
}
@@ -225,6 +215,7 @@
ac->aborted = 0;
grpc_alarm_init(&ac->alarm, deadline, on_alarm, ac, gpr_now());
+ socket->write_info.outstanding = 1;
grpc_socket_notify_on_write(socket, on_connect, ac);
return;
@@ -233,7 +224,6 @@
gpr_log(GPR_ERROR, message, utf8_message);
gpr_free(utf8_message);
if (socket) {
- socket->closed_early = 1;
grpc_winsocket_orphan(socket);
} else if (sock != INVALID_SOCKET) {
closesocket(sock);
diff --git a/src/core/iomgr/tcp_server_windows.c b/src/core/iomgr/tcp_server_windows.c
index c6137e1..b37b274 100644
--- a/src/core/iomgr/tcp_server_windows.c
+++ b/src/core/iomgr/tcp_server_windows.c
@@ -123,7 +123,6 @@
closed by the system. */
for (i = 0; i < s->nports; i++) {
server_port *sp = &s->ports[i];
- sp->socket->closed_early = 1;
grpc_winsocket_orphan(sp->socket);
}
gpr_free(s->ports);
diff --git a/src/core/iomgr/tcp_windows.c b/src/core/iomgr/tcp_windows.c
index c8483bd..2c2df00 100644
--- a/src/core/iomgr/tcp_windows.c
+++ b/src/core/iomgr/tcp_windows.c
@@ -86,12 +86,10 @@
grpc_endpoint_read_cb read_cb;
void *read_user_data;
gpr_slice read_slice;
- int outstanding_read;
grpc_endpoint_write_cb write_cb;
void *write_user_data;
gpr_slice_buffer write_slices;
- int outstanding_write;
/* The IO Completion Port runs from another thread. We need some mechanism
to protect ourselves when requesting a shutdown. */
@@ -141,14 +139,13 @@
return;
}
- GPR_ASSERT(tcp->outstanding_read);
+ GPR_ASSERT(tcp->socket->read_info.outstanding);
if (socket->read_info.wsa_error != 0) {
char *utf8_message = gpr_format_message(info->wsa_error);
gpr_log(GPR_ERROR, "ReadFile overlapped error: %s", utf8_message);
gpr_free(utf8_message);
status = GRPC_ENDPOINT_CB_ERROR;
- socket->closed_early = 1;
} else {
if (info->bytes_transfered != 0) {
sub = gpr_slice_sub(tcp->read_slice, 0, info->bytes_transfered);
@@ -161,7 +158,7 @@
}
}
- tcp->outstanding_read = 0;
+ tcp->socket->read_info.outstanding = 0;
tcp_unref(tcp);
cb(opaque, slice, nslices, status);
@@ -178,10 +175,13 @@
int error;
WSABUF buffer;
- GPR_ASSERT(!tcp->outstanding_read);
- GPR_ASSERT(!tcp->shutting_down);
+ GPR_ASSERT(!tcp->socket->read_info.outstanding);
+ if (tcp->shutting_down) {
+ cb(arg, NULL, 0, GRPC_ENDPOINT_CB_SHUTDOWN);
+ return;
+ }
tcp_ref(tcp);
- tcp->outstanding_read = 1;
+ tcp->socket->read_info.outstanding = 1;
tcp->read_cb = cb;
tcp->read_user_data = arg;
@@ -208,36 +208,6 @@
status = WSARecv(tcp->socket->socket, &buffer, 1, &bytes_read, &flags,
&info->overlapped, NULL);
- if (status == 0) {
- grpc_socket_notify_on_read(tcp->socket, on_read, tcp);
- return;
- }
-
- error = WSAGetLastError();
-
- if (error != WSA_IO_PENDING) {
- char *utf8_message = gpr_format_message(WSAGetLastError());
- gpr_log(GPR_ERROR, "WSARecv error: %s - this means we're going to leak.",
- utf8_message);
- gpr_free(utf8_message);
- /* I'm pretty sure this is a very bad situation there. Hence the log.
- What will happen now is that the socket will neither wait for read
- or write, unless the caller retry, which is unlikely, but I am not
- sure if that's guaranteed. And there might also be a write pending.
- This means that the future orphanage of that socket will be in limbo,
- and we're going to leak it. I have no idea what could cause this
- specific case however, aside from a parameter error from our call.
- Normal read errors would actually happen during the overlapped
- operation, which is the supported way to go for that. */
- tcp->outstanding_read = 0;
- tcp_unref(tcp);
- cb(arg, NULL, 0, GRPC_ENDPOINT_CB_ERROR);
- /* Per the comment above, I'm going to treat that case as a hard failure
- for now, and leave the option to catch that and debug. */
- __debugbreak();
- return;
- }
-
grpc_socket_notify_on_read(tcp->socket, on_read, tcp);
}
@@ -260,7 +230,7 @@
}
gpr_mu_unlock(&tcp->mu);
- GPR_ASSERT(tcp->outstanding_write);
+ GPR_ASSERT(tcp->socket->write_info.outstanding);
if (do_abort) {
if (from_iocp) gpr_slice_buffer_reset_and_unref(&tcp->write_slices);
@@ -274,13 +244,12 @@
gpr_log(GPR_ERROR, "WSASend overlapped error: %s", utf8_message);
gpr_free(utf8_message);
status = GRPC_ENDPOINT_CB_ERROR;
- tcp->socket->closed_early = 1;
} else {
GPR_ASSERT(info->bytes_transfered == tcp->write_slices.length);
}
gpr_slice_buffer_reset_and_unref(&tcp->write_slices);
- tcp->outstanding_write = 0;
+ tcp->socket->write_info.outstanding = 0;
tcp_unref(tcp);
cb(opaque, status);
@@ -301,11 +270,13 @@
WSABUF *allocated = NULL;
WSABUF *buffers = local_buffers;
- GPR_ASSERT(!tcp->outstanding_write);
- GPR_ASSERT(!tcp->shutting_down);
+ GPR_ASSERT(!tcp->socket->write_info.outstanding);
+ if (tcp->shutting_down) {
+ return GRPC_ENDPOINT_WRITE_ERROR;
+ }
tcp_ref(tcp);
- tcp->outstanding_write = 1;
+ tcp->socket->write_info.outstanding = 1;
tcp->write_cb = cb;
tcp->write_user_data = arg;
@@ -341,7 +312,7 @@
}
if (allocated) gpr_free(allocated);
gpr_slice_buffer_reset_and_unref(&tcp->write_slices);
- tcp->outstanding_write = 0;
+ tcp->socket->write_info.outstanding = 0;
tcp_unref(tcp);
return ret;
}
@@ -353,33 +324,6 @@
&bytes_sent, 0, &socket->write_info.overlapped, NULL);
if (allocated) gpr_free(allocated);
- /* It is possible the operation completed then. But we'd still get an IOCP
- notification. So let's ignore it and wait for the IOCP. */
- if (status != 0) {
- int error = WSAGetLastError();
- if (error != WSA_IO_PENDING) {
- char *utf8_message = gpr_format_message(WSAGetLastError());
- gpr_log(GPR_ERROR, "WSASend error: %s - this means we're going to leak.",
- utf8_message);
- gpr_free(utf8_message);
- /* I'm pretty sure this is a very bad situation there. Hence the log.
- What will happen now is that the socket will neither wait for read
- or write, unless the caller retry, which is unlikely, but I am not
- sure if that's guaranteed. And there might also be a read pending.
- This means that the future orphanage of that socket will be in limbo,
- and we're going to leak it. I have no idea what could cause this
- specific case however, aside from a parameter error from our call.
- Normal read errors would actually happen during the overlapped
- operation, which is the supported way to go for that. */
- tcp->outstanding_write = 0;
- tcp_unref(tcp);
- /* Per the comment above, I'm going to treat that case as a hard failure
- for now, and leave the option to catch that and debug. */
- __debugbreak();
- return GRPC_ENDPOINT_WRITE_ERROR;
- }
- }
-
/* As all is now setup, we can now ask for the IOCP notification. It may
trigger the callback immediately however, but no matter. */
grpc_socket_notify_on_write(socket, on_write, tcp);