[PATCH 0/2] MR210: winhttp: Use completion port for overlapped send.
There are a few issues with the way it is done now: 1. We queue overlapped WSARecv from one (user) thread and then await completion on another. As it became apparent the completions queued by a thread are canceled when thread terminates. That is currently not the case in Wine, but I have a pending MR for that: https://gitlab.winehq.org/wine/wine/-/merge_requests/135. User may use WinhttpWebSocketSend() and close the thread, if it gets to async send path that send may be canceled before it is complete. The only condition when the async operation is not canceled is that the queued async IO has a completion port and no event (which becomes the case with these patches). I tested on Windows that terminating the thread which called WinhttpWebSocketSend() doesn't cancel the async send, while I am reproducing this case with Wine and the refernced patch. 2. Using overlapped WSASend without a completion port or an event is fragile: the socket may potentially be signaled by a parallel async operation (or maybe instead not singaled until that operation completes; I think Wine and Windows currently don't agree in lot of details how that works, including maybe the higher level WSAGetOverlappedResult). This is more of theoretical point as currently we don't queue any overlapped receives. 3. WSAGetOverlappedResult is currently buggy in a funny way. It doesn't check for valid socket handle, and so if called with an overlapped without an event it waits on socket, and if socket is -1 it is current process pseudo handle and the wait hangs forever. I've sent a patch for winsock for that: https://gitlab.winehq.org/wine/wine/-/merge_requests/203. But since we need completion port for 1 anyway it also seems to also solve the potential race issue between setting socket to -1 when aborting operation and starting wait for overlapped IO on it without additional complications. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/210
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/net.c | 7 +++++++ dlls/winhttp/request.c | 4 ++-- dlls/winhttp/winhttp_private.h | 1 + 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 17520a54f63..a17582c90a6 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -51,6 +51,13 @@ static int sock_send(int fd, const void *msg, size_t len, WSAOVERLAPPED *ovr) return -1; } +BOOL netconn_wait_overlapped_result( struct netconn *conn, WSAOVERLAPPED *ovr, DWORD *len ) +{ + DWORD retflags; + + return WSAGetOverlappedResult( conn->socket, ovr, len, TRUE, &retflags ); +} + static int sock_recv(int fd, void *msg, size_t len, int flags) { int ret; diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 2754dc29163..7fb582362aa 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3336,9 +3336,9 @@ static DWORD send_frame( struct socket *socket, enum socket_opcode opcode, USHOR static DWORD complete_send_frame( struct socket *socket, WSAOVERLAPPED *ovr, const char *buf ) { - DWORD ret, retflags, len, i; + DWORD ret, len, i; - if (!WSAGetOverlappedResult( socket->request->netconn->socket, ovr, &len, TRUE, &retflags )) + if (!netconn_wait_overlapped_result( socket->request->netconn, ovr, &len )) return WSAGetLastError(); if (socket->bytes_in_send_frame_buffer) diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 98e05f068ba..6c381af627f 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -371,6 +371,7 @@ DWORD netconn_recv( struct netconn *, void *, size_t, int, int * ) DECLSPEC_HIDD DWORD netconn_resolve( WCHAR *, INTERNET_PORT, struct sockaddr_storage *, int ) DECLSPEC_HIDDEN; DWORD netconn_secure_connect( struct netconn *, WCHAR *, DWORD, CredHandle *, BOOL ) DECLSPEC_HIDDEN; DWORD netconn_send( struct netconn *, const void *, size_t, int *, WSAOVERLAPPED * ) DECLSPEC_HIDDEN; +BOOL netconn_wait_overlapped_result( struct netconn *conn, WSAOVERLAPPED *ovr, DWORD *len ) DECLSPEC_HIDDEN; void netconn_cancel_io( struct netconn *conn ) DECLSPEC_HIDDEN; DWORD netconn_set_timeout( struct netconn *, BOOL, int ) DECLSPEC_HIDDEN; BOOL netconn_is_alive( struct netconn * ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/210
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=116495 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winhttp/winhttp_private.h:371 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winhttp/winhttp_private.h:371 Task: Patch failed to apply
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/net.c | 23 +++++++++++++++++++++-- dlls/winhttp/winhttp_private.h | 1 + 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index a17582c90a6..40c9f3c8534 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -53,9 +53,20 @@ static int sock_send(int fd, const void *msg, size_t len, WSAOVERLAPPED *ovr) BOOL netconn_wait_overlapped_result( struct netconn *conn, WSAOVERLAPPED *ovr, DWORD *len ) { - DWORD retflags; + OVERLAPPED *completion_ovr; + ULONG_PTR key; - return WSAGetOverlappedResult( conn->socket, ovr, len, TRUE, &retflags ); + if (!GetQueuedCompletionStatus( conn->port, len, &key, &completion_ovr, INFINITE )) + { + WARN( "GetQueuedCompletionStatus failed, err %lu.\n", GetLastError() ); + return FALSE; + } + if ((key != conn->socket && conn->socket != -1) || completion_ovr != (OVERLAPPED *)ovr) + { + ERR( "Unexpected completion key %Ix, overlapped %p.\n", key, completion_ovr ); + return FALSE; + } + return TRUE; } static int sock_recv(int fd, void *msg, size_t len, int flags) @@ -279,6 +290,8 @@ void netconn_close( struct netconn *conn ) if (conn->socket != -1) closesocket( conn->socket ); release_host( conn->host ); + if (conn->port) + CloseHandle( conn->port ); free(conn); } @@ -452,6 +465,12 @@ DWORD netconn_send( struct netconn *conn, const void *msg, size_t len, int *sent { DWORD err; + if (ovr && !conn->port) + { + if (!(conn->port = CreateIoCompletionPort( (HANDLE)(SOCKET)conn->socket, NULL, (ULONG_PTR)conn->socket, 0 ))) + ERR( "Failed to create port.\n" ); + } + if (conn->secure) { const BYTE *ptr = msg; diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 6c381af627f..79941dee31a 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -117,6 +117,7 @@ struct netconn char *peek_msg; char *peek_msg_mem; size_t peek_len; + HANDLE port; }; struct header -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/210
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=116496 Your paranoid android. === debian11 (build log) === error: patch failed: dlls/winhttp/winhttp_private.h:371 error: patch failed: dlls/winhttp/net.c:279 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/winhttp/winhttp_private.h:371 error: patch failed: dlls/winhttp/net.c:279 Task: Patch failed to apply
participants (3)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)