[PATCH 0/7] MR195: winhttp: Abort web socket receive operations when the socket handle is closed
I think somewhat similar should happen with normal winhttp requests (I was doing some brief tests around that back then) and that should hopefully be easy to add incrementally after this patchset. Then, sends for websockets should most likely also be cancelled the same way although that is a bit harder to test as most of the time sends are processed right away before returing from WinHttpWebSocketSend (although should be possible as out of tree test which might be flaky). The patches here address cancelling web socket receive operation (including receiving web socket close status). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index fe703c19109..9abe4f45598 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -135,15 +135,16 @@ void stop_queue( struct queue *queue ) TRACE("stopped %p\n", queue); } -static struct task_header *get_next_task( struct queue *queue ) +static struct task_header *get_next_task( struct queue *queue, struct task_header *prev_task ) { struct list *entry; AcquireSRWLockExclusive( &queue->lock ); assert( queue->callback_running ); - if ((entry = list_head( &queue->queued_tasks ))) - list_remove( entry ); - else + if (prev_task) + list_remove( &prev_task->entry ); + + if (!(entry = list_head( &queue->queued_tasks ))) queue->callback_running = FALSE; ReleaseSRWLockExclusive( &queue->lock ); if (!entry) return NULL; @@ -157,12 +158,12 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx ) TRACE( "instance %p.\n", instance ); - task = get_next_task( queue ); + task = get_next_task( queue, NULL ); while (task) { task->callback( task ); /* Queue object may be freed by release_object() unless there is another task referencing it. */ - next_task = get_next_task( queue ); + next_task = get_next_task( queue, task ); release_object( task->obj ); free( task ); task = next_task; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 32 +++++++++++++++++++++++++++----- dlls/winhttp/winhttp_private.h | 1 + 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 9abe4f45598..70d727c7e2e 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -135,20 +135,41 @@ void stop_queue( struct queue *queue ) TRACE("stopped %p\n", queue); } +static void addref_task( struct task_header *task ) +{ + InterlockedIncrement( &task->refs ); +} + +static void release_task( struct task_header *task ) +{ + if (!InterlockedDecrement( &task->refs )) + free( task ); +} + static struct task_header *get_next_task( struct queue *queue, struct task_header *prev_task ) { + struct task_header *task; struct list *entry; AcquireSRWLockExclusive( &queue->lock ); assert( queue->callback_running ); if (prev_task) + { list_remove( &prev_task->entry ); - - if (!(entry = list_head( &queue->queued_tasks ))) + release_task( prev_task ); + } + if ((entry = list_head( &queue->queued_tasks ))) + { + task = LIST_ENTRY( entry, struct task_header, entry ); + addref_task( task ); + } + else + { + task = NULL; queue->callback_running = FALSE; + } ReleaseSRWLockExclusive( &queue->lock ); - if (!entry) return NULL; - return LIST_ENTRY( entry, struct task_header, entry ); + return task; } static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx ) @@ -165,7 +186,7 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx ) /* Queue object may be freed by release_object() unless there is another task referencing it. */ next_task = get_next_task( queue, task ); release_object( task->obj ); - free( task ); + release_task( task ); task = next_task; } TRACE( "instance %p exiting.\n", instance ); @@ -178,6 +199,7 @@ static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_he TRACE("queueing %p in %p\n", task_hdr, queue); task_hdr->callback = task; + task_hdr->refs = 1; task_hdr->obj = obj; addref_object( obj ); diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 4754d9258d1..ee3cdb78157 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -281,6 +281,7 @@ struct task_header struct list entry; TASK_CALLBACK callback; struct object_header *obj; + volatile LONG refs; }; struct send_request -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 70d727c7e2e..29887c8d7d0 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3839,6 +3839,25 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD return ret; } +static void socket_receive_complete( struct socket *socket, DWORD ret, WINHTTP_WEB_SOCKET_BUFFER_TYPE type, DWORD len ) +{ + if (!ret) + { + WINHTTP_WEB_SOCKET_STATUS status; + status.dwBytesTransferred = len; + status.eBufferType = type; + send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, &status, sizeof(status) ); + } + else + { + WINHTTP_WEB_SOCKET_ASYNC_RESULT result; + result.AsyncResult.dwResult = 0; + result.AsyncResult.dwError = ret; + result.Operation = WINHTTP_WEB_SOCKET_RECEIVE_OPERATION; + send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); + } +} + static void task_socket_receive( void *ctx ) { struct socket_receive *r = ctx; @@ -3850,23 +3869,7 @@ static void task_socket_receive( void *ctx ) ret = socket_receive( socket, r->buf, r->len, &count, &type ); if (receive_io_complete( socket )) - { - if (!ret) - { - WINHTTP_WEB_SOCKET_STATUS status; - status.dwBytesTransferred = count; - status.eBufferType = type; - send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, &status, sizeof(status) ); - } - else - { - WINHTTP_WEB_SOCKET_ASYNC_RESULT result; - result.AsyncResult.dwResult = API_READ_DATA; - result.AsyncResult.dwError = ret; - result.Operation = WINHTTP_WEB_SOCKET_RECEIVE_OPERATION; - send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); - } - } + socket_receive_complete( socket, ret, type, count ); } DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, DWORD *ret_len, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 108 +++++++++++++++++++++++---------- dlls/winhttp/winhttp_private.h | 3 +- 2 files changed, 78 insertions(+), 33 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 29887c8d7d0..4242551ee3b 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -182,7 +182,7 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx ) task = get_next_task( queue, NULL ); while (task) { - task->callback( task ); + task->callback( task, FALSE ); /* Queue object may be freed by release_object() unless there is another task referencing it. */ next_task = get_next_task( queue, task ); release_object( task->obj ); @@ -199,6 +199,7 @@ static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_he TRACE("queueing %p in %p\n", task_hdr, queue); task_hdr->callback = task; + task_hdr->completion_sent = 0; task_hdr->refs = 1; task_hdr->obj = obj; addref_object( obj ); @@ -225,6 +226,35 @@ static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_he return ERROR_SUCCESS; } +static BOOL task_needs_completion( struct task_header *task_hdr ) +{ + return !InterlockedExchange( &task_hdr->completion_sent, 1 ); +} + +static void cancel_queue( struct queue *queue ) +{ + struct task_header *task_hdr, *found; + + while (1) + { + AcquireSRWLockExclusive( &queue->lock ); + found = NULL; + LIST_FOR_EACH_ENTRY( task_hdr, &queue->queued_tasks, struct task_header, entry ) + { + if (task_needs_completion( task_hdr )) + { + found = task_hdr; + addref_task( found ); + break; + } + } + ReleaseSRWLockExclusive( &queue->lock ); + if (!found) break; + found->callback( found, TRUE ); + release_task( found ); + } +} + static void free_header( struct header *header ) { free( header->field ); @@ -2231,11 +2261,13 @@ end: return ret; } -static void task_send_request( void *ctx ) +static void task_send_request( void *ctx, BOOL abort ) { struct send_request *s = ctx; struct request *request = (struct request *)s->task_hdr.obj; + if (abort) return; + TRACE( "running %p\n", ctx ); send_request( request, s->headers, s->headers_len, s->optional, s->optional_len, s->total_len, s->context, TRUE ); @@ -2813,11 +2845,13 @@ static DWORD receive_response( struct request *request, BOOL async ) return ret; } -static void task_receive_response( void *ctx ) +static void task_receive_response( void *ctx, BOOL abort ) { struct receive_response *r = ctx; struct request *request = (struct request *)r->task_hdr.obj; + if (abort) return; + TRACE("running %p\n", ctx); receive_response( request, TRUE ); } @@ -2905,11 +2939,13 @@ done: return ret; } -static void task_query_data_available( void *ctx ) +static void task_query_data_available( void *ctx, BOOL abort ) { struct query_data *q = ctx; struct request *request = (struct request *)q->task_hdr.obj; + if (abort) return; + TRACE("running %p\n", ctx); query_data_available( request, q->available, TRUE ); } @@ -2956,11 +2992,13 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) return !ret || ret == ERROR_IO_PENDING; } -static void task_read_data( void *ctx ) +static void task_read_data( void *ctx, BOOL abort ) { struct read_data *r = ctx; struct request *request = (struct request *)r->task_hdr.obj; + if (abort) return; + TRACE("running %p\n", ctx); read_data( request, r->buffer, r->to_read, r->read, TRUE ); } @@ -3031,11 +3069,13 @@ static DWORD write_data( struct request *request, const void *buffer, DWORD to_w return ret; } -static void task_write_data( void *ctx ) +static void task_write_data( void *ctx, BOOL abort ) { struct write_data *w = ctx; struct request *request = (struct request *)w->task_hdr.obj; + if (abort) return; + TRACE("running %p\n", ctx); write_data( request, w->buffer, w->to_write, w->written, TRUE ); } @@ -3315,13 +3355,10 @@ static void send_io_complete( struct object_header *hdr ) } /* returns FALSE if sending callback should be omitted. */ -static BOOL receive_io_complete( struct socket *socket ) +static void receive_io_complete( struct socket *socket ) { LONG count = InterlockedDecrement( &socket->hdr.pending_receives ); - assert( count >= 0 || socket->state == SOCKET_STATE_CLOSED); - /* count is reset to zero during websocket close so if count went negative - * then WinHttpWebSocketClose() is to send the callback. */ - return count >= 0; + assert( count >= 0 ); } static BOOL socket_can_send( struct socket *socket ) @@ -3428,12 +3465,14 @@ static DWORD socket_send( struct socket *socket, WINHTTP_WEB_SOCKET_BUFFER_TYPE return send_frame( socket, opcode, 0, buf, len, final, ovr ); } -static void task_socket_send( void *ctx ) +static void task_socket_send( void *ctx, BOOL abort ) { struct socket_send *s = ctx; struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; + if (abort) return; + TRACE("running %p\n", ctx); if (s->complete_async) ret = complete_send_frame( socket, &s->ovr, s->buf ); @@ -3623,11 +3662,13 @@ static DWORD receive_frame( struct socket *socket, DWORD *ret_len, enum socket_o return ERROR_SUCCESS; } -static void task_socket_send_pong( void *ctx ) +static void task_socket_send_pong( void *ctx, BOOL abort ) { struct socket_send *s = ctx; struct socket *socket = (struct socket *)s->task_hdr.obj; + if (abort) return; + TRACE("running %p\n", ctx); if (s->complete_async) complete_send_frame( socket, &s->ovr, NULL ); @@ -3858,17 +3899,23 @@ static void socket_receive_complete( struct socket *socket, DWORD ret, WINHTTP_W } } -static void task_socket_receive( void *ctx ) +static void task_socket_receive( void *ctx, BOOL abort ) { struct socket_receive *r = ctx; struct socket *socket = (struct socket *)r->task_hdr.obj; DWORD ret, count; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; + if (abort) + { + socket_receive_complete( socket, ERROR_WINHTTP_OPERATION_CANCELLED, 0, 0 ); + return; + } + TRACE("running %p\n", ctx); ret = socket_receive( socket, r->buf, r->len, &count, &type ); - - if (receive_io_complete( socket )) + receive_io_complete( socket ); + if (task_needs_completion( &r->task_hdr )) socket_receive_complete( socket, ret, type, count ); } @@ -3939,12 +3986,14 @@ static void socket_shutdown_complete( struct socket *socket, DWORD ret ) } } -static void task_socket_shutdown( void *ctx ) +static void task_socket_shutdown( void *ctx, BOOL abort ) { struct socket_shutdown *s = ctx; struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; + if (abort) return; + TRACE("running %p\n", ctx); if (s->complete_async) ret = complete_send_frame( socket, &s->ovr, s->reason ); @@ -4073,15 +4122,18 @@ static void socket_close_complete( struct socket *socket, DWORD ret ) } } -static void task_socket_close( void *ctx ) +static void task_socket_close( void *ctx, BOOL abort ) { struct socket_shutdown *s = ctx; struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; + if (abort) return; + TRACE("running %p\n", ctx); ret = socket_close( socket ); + receive_io_complete( socket ); socket_close_complete( socket, ret ); } @@ -4113,25 +4165,14 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) { - /* When closing the socket pending receives are cancelled. Setting socket->hdr.pending_receives to zero - * will prevent pending receives from sending callbacks. */ - pending_receives = InterlockedExchange( &socket->hdr.pending_receives, 0 ); - assert( pending_receives >= 0 ); - if (pending_receives) - { - WINHTTP_WEB_SOCKET_ASYNC_RESULT result; - - result.AsyncResult.dwResult = 0; - result.AsyncResult.dwError = ERROR_WINHTTP_OPERATION_CANCELLED; - result.Operation = WINHTTP_WEB_SOCKET_RECEIVE_OPERATION; - send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); - } + pending_receives = InterlockedIncrement( &socket->hdr.pending_receives ); + cancel_queue( &socket->recv_q ); } if (prev_state < SOCKET_STATE_SHUTDOWN && (ret = send_socket_shutdown( socket, status, reason, len, FALSE ))) goto done; - if (!pending_receives && socket->close_frame_received) + if (pending_receives == 1 && socket->close_frame_received) { if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) socket_close_complete( socket, socket->close_frame_receive_err ); @@ -4144,7 +4185,10 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas if (!(s = calloc( 1, sizeof(*s) ))) return FALSE; if ((ret = queue_task( &socket->recv_q, task_socket_close, &s->task_hdr, &socket->hdr ))) + { + InterlockedDecrement( &socket->hdr.pending_receives ); free( s ); + } } else ret = socket_close( socket ); diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index ee3cdb78157..17102107c77 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -274,7 +274,7 @@ struct socket BOOL last_receive_final; }; -typedef void (*TASK_CALLBACK)( void *ctx ); +typedef void (*TASK_CALLBACK)( void *ctx, BOOL abort ); struct task_header { @@ -282,6 +282,7 @@ struct task_header TASK_CALLBACK callback; struct object_header *obj; volatile LONG refs; + volatile LONG completion_sent; }; struct send_request -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/handle.c | 7 ++++++- dlls/winhttp/net.c | 10 +++++++++- dlls/winhttp/request.c | 27 ++++++++++++++++++++++++++- dlls/winhttp/session.c | 3 +++ dlls/winhttp/winhttp_private.h | 2 ++ 5 files changed, 46 insertions(+), 3 deletions(-) diff --git a/dlls/winhttp/handle.c b/dlls/winhttp/handle.c index 38959fd6456..33a4cd6bb71 100644 --- a/dlls/winhttp/handle.c +++ b/dlls/winhttp/handle.c @@ -142,7 +142,12 @@ BOOL free_handle( HINTERNET hinternet ) LeaveCriticalSection( &handle_cs ); - if (hdr) release_object( hdr ); + if (hdr) + { + if (hdr->vtbl->handle_closing) + hdr->vtbl->handle_closing( hdr ); + release_object( hdr ); + } EnterCriticalSection( &handle_cs ); if (next_handle > handle && !handles[handle]) next_handle = handle; diff --git a/dlls/winhttp/net.c b/dlls/winhttp/net.c index 07a65c5465f..17520a54f63 100644 --- a/dlls/winhttp/net.c +++ b/dlls/winhttp/net.c @@ -269,7 +269,8 @@ void netconn_close( struct netconn *conn ) free(conn->extra_buf); DeleteSecurityContext(&conn->ssl_ctx); } - closesocket( conn->socket ); + if (conn->socket != -1) + closesocket( conn->socket ); release_host( conn->host ); free(conn); } @@ -629,6 +630,13 @@ DWORD netconn_recv( struct netconn *conn, void *buf, size_t len, int flags, int return ERROR_SUCCESS; } +void netconn_cancel_io( struct netconn *conn ) +{ + SOCKET socket = InterlockedExchange( (LONG *)&conn->socket, -1 ); + + closesocket( socket ); +} + ULONG netconn_query_data_available( struct netconn *conn ) { return conn->secure ? conn->peek_len : 0; diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 4242551ee3b..3a239966c69 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -255,6 +255,16 @@ static void cancel_queue( struct queue *queue ) } } +static BOOL is_queue_empty( struct queue *queue ) +{ + BOOL ret; + + AcquireSRWLockExclusive( &queue->lock ); + ret = list_empty( &queue->queued_tasks ); + ReleaseSRWLockExclusive( &queue->lock ); + return ret; +} + static void free_header( struct header *header ) { free( header->field ); @@ -1885,7 +1895,8 @@ static void finished_reading( struct request *request ) if (!request->netconn) return; - if (request->hdr.disable_flags & WINHTTP_DISABLE_KEEP_ALIVE) close = TRUE; + if (request->netconn->socket == -1) close = TRUE; + else if (request->hdr.disable_flags & WINHTTP_DISABLE_KEEP_ALIVE) close = TRUE; else if (!query_headers( request, WINHTTP_QUERY_CONNECTION, NULL, connection, &size, NULL ) || !query_headers( request, WINHTTP_QUERY_PROXY_CONNECTION, NULL, connection, &size, NULL )) { @@ -3121,6 +3132,19 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w return !ret; } +static void socket_handle_closing( struct object_header *hdr ) +{ + struct socket *socket = (struct socket *)hdr; + + if (socket->request->netconn->secure) + { + cancel_queue( &socket->send_q ); + cancel_queue( &socket->recv_q ); + } + if (!is_queue_empty( &socket->send_q ) || !is_queue_empty( &socket->recv_q )) + netconn_cancel_io( socket->request->netconn ); +} + static BOOL socket_query_option( struct object_header *hdr, DWORD option, void *buffer, DWORD *buflen ) { FIXME( "unimplemented option %lu\n", option ); @@ -3151,6 +3175,7 @@ static BOOL socket_set_option( struct object_header *hdr, DWORD option, void *bu static const struct object_vtbl socket_vtbl = { + socket_handle_closing, socket_destroy, socket_query_option, socket_set_option, diff --git a/dlls/winhttp/session.c b/dlls/winhttp/session.c index 3b00d2448d1..01e95895eb3 100644 --- a/dlls/winhttp/session.c +++ b/dlls/winhttp/session.c @@ -242,6 +242,7 @@ static BOOL session_set_option( struct object_header *hdr, DWORD option, void *b static const struct object_vtbl session_vtbl = { + NULL, session_destroy, session_query_option, session_set_option @@ -382,6 +383,7 @@ static BOOL connect_query_option( struct object_header *hdr, DWORD option, void static const struct object_vtbl connect_vtbl = { + NULL, connect_destroy, connect_query_option, NULL @@ -1085,6 +1087,7 @@ static BOOL request_set_option( struct object_header *hdr, DWORD option, void *b static const struct object_vtbl request_vtbl = { + NULL, request_destroy, request_query_option, request_set_option diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 17102107c77..98e05f068ba 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -30,6 +30,7 @@ struct object_header; struct object_vtbl { + void (*handle_closing) ( struct object_header * ); void (*destroy)( struct object_header * ); BOOL (*query_option)( struct object_header *, DWORD, void *, DWORD * ); BOOL (*set_option)( struct object_header *, DWORD, void *, DWORD ); @@ -370,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; +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; const void *netconn_get_certificate( struct netconn * ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 3a239966c69..397421ebcd0 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -4153,13 +4153,18 @@ static void task_socket_close( void *ctx, BOOL abort ) struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; - if (abort) return; + if (abort) + { + socket_close_complete( socket, ERROR_WINHTTP_OPERATION_CANCELLED ); + return; + } TRACE("running %p\n", ctx); ret = socket_close( socket ); receive_io_complete( socket ); - socket_close_complete( socket, ret ); + if (task_needs_completion( &s->task_hdr )) + socket_close_complete( socket, ret ); } DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reason, DWORD len ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/tests/notification.c | 238 ++++++++++++++++++++++++++++-- 1 file changed, 229 insertions(+), 9 deletions(-) diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 50874a49a07..7f2f741fef5 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -59,11 +59,12 @@ struct notification DWORD flags; /* a combination of NF_* flags */ }; -#define NF_ALLOW 0x0001 /* notification may or may not happen */ -#define NF_WINE_ALLOW 0x0002 /* wine sends notification when it should not */ -#define NF_SIGNAL 0x0004 /* signal wait handle when notified */ -#define NF_MAIN_THREAD 0x0008 /* the operation completes synchronously and callback is called from the main thread */ -#define NF_SAVE_BUFFER 0x0010 /* save buffer data when notified */ +#define NF_ALLOW 0x0001 /* notification may or may not happen */ +#define NF_WINE_ALLOW 0x0002 /* wine sends notification when it should not */ +#define NF_SIGNAL 0x0004 /* signal wait handle when notified */ +#define NF_MAIN_THREAD 0x0008 /* the operation completes synchronously and callback is called from the main thread */ +#define NF_OTHER_THREAD 0x0010 /* callback is called from the other thread */ +#define NF_SAVE_BUFFER 0x0020 /* save buffer data when notified */ struct info { @@ -121,6 +122,11 @@ static void CALLBACK check_notification( HINTERNET handle, DWORD_PTR context, DW ok(GetCurrentThreadId() == info->main_thread_id, "%u: expected callback to be called from the same thread\n", info->line); } + if (info->test[info->index].flags & NF_OTHER_THREAD) + { + ok(GetCurrentThreadId() != info->main_thread_id, "%u: expected callback to be called from the other thread\n", + info->line); + } if (info->test[info->index].flags & NF_SAVE_BUFFER) { info->buflen = buflen; @@ -704,14 +710,63 @@ static const struct notification websocket_test2[] = { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED, NF_SIGNAL }, { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SIGNAL }, { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_MAIN_THREAD | NF_SAVE_BUFFER}, - { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_SIGNAL }, - { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING }, + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_CLOSE_COMPLETE, NF_SIGNAL | NF_OTHER_THREAD }, + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_MAIN_THREAD }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL } }; -static const struct notification websocket_test3[] = +static struct notification websocket_test3[] = +{ + { winhttp_open_request, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_RESOLVING_NAME, NF_ALLOW }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_NAME_RESOLVED, NF_ALLOW }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTING_TO_SERVER }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTED_TO_SERVER }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_SENDING_REQUEST }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE, NF_SIGNAL }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE, NF_SIGNAL }, + { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED, NF_SIGNAL }, + { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SIGNAL }, + + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_MAIN_THREAD }, + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_SAVE_BUFFER }, + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL | NF_OTHER_THREAD }, + + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL | NF_MAIN_THREAD }, +}; + +static struct notification websocket_test4[] = +{ + { winhttp_open_request, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_RESOLVING_NAME, NF_ALLOW }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_NAME_RESOLVED, NF_ALLOW }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTING_TO_SERVER }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_CONNECTED_TO_SERVER }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_SENDING_REQUEST }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_REQUEST_SENT }, + { winhttp_send_request, WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE, NF_SIGNAL }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_RESPONSE_RECEIVED }, + { winhttp_receive_response, WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE, NF_SIGNAL }, + { winhttp_websocket_complete_upgrade, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED, NF_SIGNAL }, + { winhttp_websocket_receive, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, NF_SIGNAL }, + + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, NF_SAVE_BUFFER }, + { winhttp_websocket_close, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL | NF_OTHER_THREAD }, + + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_CONNECTION_CLOSED, NF_WINE_ALLOW }, + { winhttp_close_handle, WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING, NF_SIGNAL/* | NF_MAIN_THREAD*/ }, +}; + +static const struct notification websocket_test5[] = { { winhttp_open_request, WINHTTP_CALLBACK_STATUS_HANDLE_CREATED }, { winhttp_send_request, WINHTTP_CALLBACK_STATUS_RESOLVING_NAME, NF_ALLOW }, @@ -1065,11 +1120,176 @@ static void test_websocket(BOOL secure) WaitForSingleObject( info.wait, INFINITE ); end_test( &info, __LINE__ ); - /* Test socket shutdown while receive is pending. */ + + /* Test socket handle close while web socket close is pending. */ info.test = websocket_test3; info.count = ARRAY_SIZE( websocket_test3 ); info.index = 0; + for (i = 0; websocket_test3[i].function != winhttp_websocket_close; ++i) + ; + + if (secure) + websocket_test3[i + 1].flags = (websocket_test3[i + 1].flags & ~NF_OTHER_THREAD) | NF_MAIN_THREAD; + else + websocket_test3[i + 1].flags = (websocket_test3[i + 1].flags & ~NF_MAIN_THREAD) | NF_OTHER_THREAD; + + setup_test( &info, winhttp_open_request, __LINE__ ); + request = WinHttpOpenRequest( connection, NULL, L"/", NULL, NULL, NULL, secure ? WINHTTP_FLAG_SECURE : 0); + ok( request != NULL, "got %lu\n", err ); + + if (secure) + { + flags = SECURITY_FLAG_IGNORE_UNKNOWN_CA | SECURITY_FLAG_IGNORE_CERT_DATE_INVALID | + SECURITY_FLAG_IGNORE_CERT_CN_INVALID; + ret = WinHttpSetOption(request, WINHTTP_OPTION_SECURITY_FLAGS, &flags, sizeof(flags)); + ok( ret, "failed to set security flags %lu\n", GetLastError() ); + } + + ret = WinHttpSetOption( request, WINHTTP_OPTION_UPGRADE_TO_WEB_SOCKET, NULL, 0 ); + ok( ret, "got %lu\n", GetLastError() ); + + setup_test( &info, winhttp_send_request, __LINE__ ); + ret = WinHttpSendRequest( request, NULL, 0, NULL, 0, 0, 0 ); + ok( ret, "got %lu\n", GetLastError() ); + WaitForSingleObject( info.wait, INFINITE ); + + setup_test( &info, winhttp_receive_response, __LINE__ ); + ret = WinHttpReceiveResponse( request, NULL ); + ok( ret, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + + size = sizeof(status); + ret = WinHttpQueryHeaders( request, WINHTTP_QUERY_STATUS_CODE|WINHTTP_QUERY_FLAG_NUMBER, NULL, &status, &size, NULL ); + ok( ret, "failed unexpectedly %lu\n", err ); + ok( status == 101, "got %lu\n", status ); + + setup_test( &info, winhttp_websocket_complete_upgrade, __LINE__ ); + socket = pWinHttpWebSocketCompleteUpgrade( request, (DWORD_PTR)context ); + ok( socket != NULL, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + + setup_test( &info, winhttp_websocket_receive, __LINE__ ); + buffer[0] = 0; + err = pWinHttpWebSocketReceive( socket, buffer, sizeof(buffer), &size, &type ); + ok( err == ERROR_SUCCESS, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + ok( buffer[0] == 'R', "unexpected data\n" ); + + setup_test( &info, winhttp_websocket_close, __LINE__ ); + + err = pWinHttpWebSocketReceive( socket, buffer, sizeof(buffer), &size, &type ); + ok( err == ERROR_SUCCESS, "got %lu\n", err ); + + err = pWinHttpWebSocketClose( socket, 1000, (void *)"success", sizeof("success") ); + ok( err == ERROR_SUCCESS, "got %lu\n", err ); + + info.buflen = 0xdeadbeef; + WinHttpCloseHandle( socket ); + WaitForSingleObject( info.wait, INFINITE ); + + ok( info.buflen == sizeof(*result), "got %u\n", info.buflen ); + result = (WINHTTP_WEB_SOCKET_ASYNC_RESULT *)info.buffer; + ok( result->Operation == WINHTTP_WEB_SOCKET_CLOSE_OPERATION, "got %u\n", result->Operation ); + todo_wine ok( !result->AsyncResult.dwResult, "got %Iu\n", result->AsyncResult.dwResult ); + todo_wine_if( !secure ) + ok( result->AsyncResult.dwError == ERROR_WINHTTP_OPERATION_CANCELLED, "got %lu\n", result->AsyncResult.dwError ); + + setup_test( &info, winhttp_close_handle, __LINE__ ); + WinHttpCloseHandle( request ); + WaitForSingleObject( info.wait, INFINITE ); + end_test( &info, __LINE__ ); + + /* Test socket handle close while receive is pending. */ + info.test = websocket_test4; + info.count = ARRAY_SIZE( websocket_test4 ); + info.index = 0; + + for (i = 0; websocket_test4[i].function != winhttp_websocket_close; ++i) + ; + + if (secure) + { + /* Sometimes (rarely) Windows calls the callback from the async receive thread, so don't set + * NF_MAIN_THREAD here to avoid flaky test. Maybe it cancels the socket IO first and aborts the + * completion next so sometimes the async thread is fast enough to send the notification + * before it is done by the main thread. */ + websocket_test4[i].flags = (websocket_test4[i].flags & ~NF_OTHER_THREAD)/* | NF_MAIN_THREAD*/; + } + else + { + websocket_test4[i].flags = (websocket_test4[i].flags & ~NF_MAIN_THREAD) | NF_OTHER_THREAD; + } + + setup_test( &info, winhttp_open_request, __LINE__ ); + request = WinHttpOpenRequest( connection, NULL, L"/", NULL, NULL, NULL, secure ? WINHTTP_FLAG_SECURE : 0); + ok( request != NULL, "got %lu\n", err ); + + if (secure) + { + flags = SECURITY_FLAG_IGNORE_UNKNOWN_CA | SECURITY_FLAG_IGNORE_CERT_DATE_INVALID | + SECURITY_FLAG_IGNORE_CERT_CN_INVALID; + ret = WinHttpSetOption(request, WINHTTP_OPTION_SECURITY_FLAGS, &flags, sizeof(flags)); + ok( ret, "failed to set security flags %lu\n", GetLastError() ); + } + + ret = WinHttpSetOption( request, WINHTTP_OPTION_UPGRADE_TO_WEB_SOCKET, NULL, 0 ); + ok( ret, "got %lu\n", GetLastError() ); + + setup_test( &info, winhttp_send_request, __LINE__ ); + ret = WinHttpSendRequest( request, NULL, 0, NULL, 0, 0, 0 ); + ok( ret, "got %lu\n", GetLastError() ); + WaitForSingleObject( info.wait, INFINITE ); + + setup_test( &info, winhttp_receive_response, __LINE__ ); + ret = WinHttpReceiveResponse( request, NULL ); + ok( ret, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + + size = sizeof(status); + ret = WinHttpQueryHeaders( request, WINHTTP_QUERY_STATUS_CODE|WINHTTP_QUERY_FLAG_NUMBER, NULL, &status, &size, NULL ); + ok( ret, "failed unexpectedly %lu\n", err ); + ok( status == 101, "got %lu\n", status ); + + setup_test( &info, winhttp_websocket_complete_upgrade, __LINE__ ); + socket = pWinHttpWebSocketCompleteUpgrade( request, (DWORD_PTR)context ); + ok( socket != NULL, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + + setup_test( &info, winhttp_websocket_receive, __LINE__ ); + buffer[0] = 0; + err = pWinHttpWebSocketReceive( socket, buffer, sizeof(buffer), &size, &type ); + ok( err == ERROR_SUCCESS, "got %lu\n", err ); + WaitForSingleObject( info.wait, INFINITE ); + ok( buffer[0] == 'R', "unexpected data\n" ); + + setup_test( &info, winhttp_websocket_close, __LINE__ ); + + info.buflen = 0xdeadbeef; + + err = pWinHttpWebSocketReceive( socket, buffer, sizeof(buffer), &size, &type ); + ok( err == ERROR_SUCCESS, "got %lu\n", err ); + + WinHttpCloseHandle( socket ); + WaitForSingleObject( info.wait, INFINITE ); + + ok( info.buflen == sizeof(*result), "got %u\n", info.buflen ); + result = (WINHTTP_WEB_SOCKET_ASYNC_RESULT *)info.buffer; + ok( result->Operation == WINHTTP_WEB_SOCKET_RECEIVE_OPERATION, "got %u\n", result->Operation ); + ok( !result->AsyncResult.dwResult, "got %Iu\n", result->AsyncResult.dwResult ); + todo_wine_if( !secure ) + ok( result->AsyncResult.dwError == ERROR_WINHTTP_OPERATION_CANCELLED, "got %lu\n", result->AsyncResult.dwError ); + + setup_test( &info, winhttp_close_handle, __LINE__ ); + WinHttpCloseHandle( request ); + WaitForSingleObject( info.wait, INFINITE ); + end_test( &info, __LINE__ ); + + /* Test socket shutdown while receive is pending. */ + info.test = websocket_test5; + info.count = ARRAY_SIZE( websocket_test5 ); + info.index = 0; + setup_test( &info, winhttp_open_request, __LINE__ ); request = WinHttpOpenRequest( connection, NULL, L"/", NULL, NULL, NULL, secure ? WINHTTP_FLAG_SECURE : 0); ok( request != NULL, "got %lu\n", err ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/195
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=116370 Your paranoid android. === w1064 (64 bit report) === winhttp: notification.c:122: Test failed: 1198: expected callback to be called from the same thread === w1064_adm (64 bit report) === winhttp: notification.c:122: Test failed: 1179: expected callback to be called from the same thread === w10pro64_ja (64 bit report) === winhttp: notification.c:122: Test failed: 1179: expected callback to be called from the same thread === w10pro64_zh_CN (64 bit report) === winhttp: notification.c:122: Test failed: 1179: expected callback to be called from the same thread === debian11 (32 bit German report) === winhttp: notification.c:127: Test failed: 1266: expected callback to be called from the other thread
On 6/6/22 23:02, Paul Gofman (@gofman) wrote:
I think somewhat similar should happen with normal winhttp requests (I was doing some brief tests around that back then) and that should hopefully be easy to add incrementally after this patchset.
Then, sends for websockets should most likely also be cancelled the same way although that is a bit harder to test as most of the time sends are processed right away before returing from WinHttpWebSocketSend (although should be possible as out of tree test which might be flaky).
The patches here address cancelling web socket receive operation (including receiving web socket close status).
Eh, I am missing sign-off and also test in notification.c line 1179 is flaky (would comment out the other thread requirement, the same comment as for the next nest on line 1213 applies), but I will probably wait for some initial feedback before resending the whole series.
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
return !ret; }
+static void socket_handle_closing( struct object_header *hdr ) +{ + struct socket *socket = (struct socket *)hdr; + + if (socket->request->netconn->secure) + { + cancel_queue( &socket->send_q ); + cancel_queue( &socket->recv_q ); + } + if (!is_queue_empty( &socket->send_q ) || !is_queue_empty( &socket->recv_q )) + netconn_cancel_io( socket->request->netconn ); +}
I doesn't make a lot of sense that async behavior depends on the connection being secure. Do we really need to replicate this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/195#note_1639
On 7 Jun 2022, at 07:54, Hans Leidekker (@hans) <wine(a)gitlab.winehq.org> wrote:
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
return !ret; }
+static void socket_handle_closing( struct object_header *hdr ) +{ + struct socket *socket = (struct socket *)hdr; + + if (socket->request->netconn->secure) + { + cancel_queue( &socket->send_q ); + cancel_queue( &socket->recv_q ); + } + if (!is_queue_empty( &socket->send_q ) || !is_queue_empty( &socket->recv_q )) + netconn_cancel_io( socket->request->netconn ); +}
I doesn't make a lot of sense that async behavior depends on the connection being secure. Do we really need to replicate this?
I don’t have any examples of apps which are happy with one way of doing it and not another. I’d suggest to keep ‘secure’ variant then as I think secure connection is used more often. Also, this variant should deliver callbacks earlier (before exiting close handle call, except for handle closing notification) and apps are more likely to depend on that (e. g. a game where I saw crash on exit without these patches seems to have a race and not crashing depends on all the notifications delivered during WinHttpHandleClose or very shortly after; it also uses secure connection).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/195#note_1639
On Tue, 2022-06-07 at 09:05 -0500, Paul Gofman wrote:
On 7 Jun 2022, at 07:54, Hans Leidekker (@hans) <wine(a)gitlab.winehq.org> wrote:
Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
return !ret; }
+static void socket_handle_closing( struct object_header *hdr ) +{ + struct socket *socket = (struct socket *)hdr; + + if (socket->request->netconn->secure) + { + cancel_queue( &socket->send_q ); + cancel_queue( &socket->recv_q ); + } + if (!is_queue_empty( &socket->send_q ) || !is_queue_empty( &socket->recv_q )) + netconn_cancel_io( socket->request->netconn ); +} I doesn't make a lot of sense that async behavior depends on the connection being secure. Do we really need to replicate this?
I don’t have any examples of apps which are happy with one way of doing it and not another. I’d suggest to keep ‘secure’ variant then as I think secure connection is used more often. Also, this variant should deliver callbacks earlier (before exiting close handle call, except for handle closing notification) and apps are more likely to depend on that (e. g. a game where I saw crash on exit without these patches seems to have a race and not crashing depends on all the notifications delivered during WinHttpHandleClose or very shortly after; it also uses secure connection).
I agree. The other patches look good.
participants (6)
-
Hans Leidekker -
Hans Leidekker (＠hans) -
Marvin -
Paul Gofman -
Paul Gofman
-
Paul Gofman (＠gofman)