[PATCH 1/3] winhttp: Use a single thread pool wrapper callback for async tasks.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- The primary motivation under this patchset is to avoid leaking threads when an application is leaking winhttp handles by using default thread pool (like Monster Hunter Rise which was failing due to consequences of that). Also, preparatory patches 1-2 remove some code duplication around referencing and releasing queued task related objects. As a separate note, we are probably not doing quite the right thing on closing request and socket handle (as far as my brief testing of that shows (at least with websocket handle) it is probably supposed to behave somewhat similar to WinHttpWebSocketClose(): abort the queued operation and send cancel status immediately. If we need to fix that in the future it is probably a bit easier to do with the handle's object reference being managed in a single place. dlls/winhttp/request.c | 74 +++++++++++++++++++--------------- dlls/winhttp/winhttp_private.h | 15 +++++++ 2 files changed, 56 insertions(+), 33 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index b409c4cdced..36a3bb909b9 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -146,15 +146,23 @@ void stop_queue( struct queue *queue ) TRACE("stopped %p\n", queue); } -static DWORD queue_task( struct queue *queue, PTP_WORK_CALLBACK task, void *ctx ) +static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +{ + struct task_header *task_hdr = ctx; + + task_hdr->callback( task_hdr ); +} + +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr ) { TP_WORK *work; DWORD ret; if ((ret = start_queue( queue ))) return ret; - if (!(work = CreateThreadpoolWork( task, ctx, &queue->env ))) return GetLastError(); - TRACE("queueing %p in %p\n", work, queue); + if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError(); + TRACE("queueing %p in %p\n", task_hdr, queue); + task_hdr->callback = task; SubmitThreadpoolWork( work ); CloseThreadpoolWork( work ); @@ -2167,11 +2175,11 @@ end: return ret; } -static void CALLBACK task_send_request( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_send_request( void *ctx ) { struct send_request *s = ctx; - TRACE("running %p\n", work); + TRACE( "running %p\n", ctx ); send_request( s->request, s->headers, s->headers_len, s->optional, s->optional_len, s->total_len, s->context, TRUE ); release_object( &s->request->hdr ); @@ -2219,7 +2227,7 @@ BOOL WINAPI WinHttpSendRequest( HINTERNET hrequest, const WCHAR *headers, DWORD s->context = context; addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_send_request, s ))) + if ((ret = queue_task( &request->queue, task_send_request, &s->task_hdr ))) { release_object( &request->hdr ); free( s->headers ); @@ -2753,11 +2761,11 @@ static DWORD receive_response( struct request *request, BOOL async ) return ret; } -static void CALLBACK task_receive_response( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_receive_response( void *ctx ) { struct receive_response *r = ctx; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); receive_response( r->request, TRUE ); release_object( &r->request->hdr ); @@ -2794,7 +2802,7 @@ BOOL WINAPI WinHttpReceiveResponse( HINTERNET hrequest, LPVOID reserved ) r->request = request; addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_receive_response, r ))) + if ((ret = queue_task( &request->queue, task_receive_response, &r->task_hdr ))) { release_object( &request->hdr ); free( r ); @@ -2852,11 +2860,11 @@ done: return ret; } -static void CALLBACK task_query_data_available( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_query_data_available( void *ctx ) { struct query_data *q = ctx; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); query_data_available( q->request, q->available, TRUE ); release_object( &q->request->hdr ); @@ -2895,7 +2903,7 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) q->available = available; addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_query_data_available, q ))) + if ((ret = queue_task( &request->queue, task_query_data_available, &q->task_hdr ))) { release_object( &request->hdr ); free( q ); @@ -2909,11 +2917,11 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) return !ret || ret == ERROR_IO_PENDING; } -static void CALLBACK task_read_data( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_read_data( void *ctx ) { struct read_data *r = ctx; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); read_data( r->request, r->buffer, r->to_read, r->read, TRUE ); release_object( &r->request->hdr ); @@ -2954,7 +2962,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW r->read = read; addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_read_data, r ))) + if ((ret = queue_task( &request->queue, task_read_data, &r->task_hdr ))) { release_object( &request->hdr ); free( r ); @@ -2990,11 +2998,11 @@ static DWORD write_data( struct request *request, const void *buffer, DWORD to_w return ret; } -static void CALLBACK task_write_data( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_write_data( void *ctx ) { struct write_data *w = ctx; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); write_data( w->request, w->buffer, w->to_write, w->written, TRUE ); release_object( &w->request->hdr ); @@ -3034,7 +3042,7 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w w->written = written; addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_write_data, w ))) + if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr ))) { release_object( &request->hdr ); free( w ); @@ -3392,12 +3400,12 @@ static DWORD socket_send( struct socket *socket, WINHTTP_WEB_SOCKET_BUFFER_TYPE return send_frame( socket, opcode, 0, buf, len, final, ovr ); } -static void CALLBACK task_socket_send( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_socket_send( void *ctx ) { struct socket_send *s = ctx; DWORD ret; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); if (s->complete_async) ret = complete_send_frame( s->socket, &s->ovr, s->buf ); else ret = socket_send( s->socket, s->type, s->buf, s->len, NULL ); @@ -3479,7 +3487,7 @@ DWORD WINAPI WinHttpWebSocketSend( HINTERNET hsocket, WINHTTP_WEB_SOCKET_BUFFER_ s->len = len; addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_send, s ))) + if ((ret = queue_task( &socket->send_q, task_socket_send, &s->task_hdr ))) { InterlockedDecrement( &socket->hdr.pending_sends ); InterlockedExchange( &socket->pending_noncontrol_send, 0 ); @@ -3596,11 +3604,11 @@ static DWORD receive_frame( struct socket *socket, DWORD *ret_len, enum socket_o return ERROR_SUCCESS; } -static void CALLBACK task_socket_send_pong( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_socket_send_pong( void *ctx ) { struct socket_send *s = ctx; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); if (s->complete_async) complete_send_frame( s->socket, &s->ovr, NULL ); else send_frame( s->socket, SOCKET_OPCODE_PONG, 0, NULL, 0, TRUE, NULL ); @@ -3638,7 +3646,7 @@ static DWORD socket_send_pong( struct socket *socket ) s->complete_async = complete_async; s->socket = socket; addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_send_pong, s ))) + if ((ret = queue_task( &socket->send_q, task_socket_send_pong, &s->task_hdr ))) { InterlockedDecrement( &socket->hdr.pending_sends ); release_object( &socket->hdr ); @@ -3817,13 +3825,13 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD return ret; } -static void CALLBACK task_socket_receive( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_socket_receive( void *ctx ) { struct socket_receive *r = ctx; DWORD ret, count; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); ret = socket_receive( r->socket, r->buf, r->len, &count, &type ); if (receive_io_complete( r->socket )) @@ -3893,7 +3901,7 @@ DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, D r->len = len; addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->recv_q, task_socket_receive, r ))) + if ((ret = queue_task( &socket->recv_q, task_socket_receive, &r->task_hdr ))) { InterlockedDecrement( &socket->hdr.pending_receives ); release_object( &socket->hdr ); @@ -3919,12 +3927,12 @@ static void socket_shutdown_complete( struct socket *socket, DWORD ret ) } } -static void CALLBACK task_socket_shutdown( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_socket_shutdown( void *ctx ) { struct socket_shutdown *s = ctx; DWORD ret; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); if (s->complete_async) ret = complete_send_frame( s->socket, &s->ovr, s->reason ); else ret = send_frame( s->socket, SOCKET_OPCODE_CLOSE, s->status, s->reason, s->len, TRUE, NULL ); @@ -3971,7 +3979,7 @@ static DWORD send_socket_shutdown( struct socket *socket, USHORT status, const v s->send_callback = send_callback; addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_shutdown, s ))) + if ((ret = queue_task( &socket->send_q, task_socket_shutdown, &s->task_hdr ))) { InterlockedDecrement( &socket->hdr.pending_sends ); release_object( &socket->hdr ); @@ -4057,12 +4065,12 @@ static void socket_close_complete( struct socket *socket, DWORD ret ) } } -static void CALLBACK task_socket_close( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static void task_socket_close( void *ctx ) { struct socket_shutdown *s = ctx; DWORD ret; - TRACE("running %p\n", work); + TRACE("running %p\n", ctx); ret = socket_close( s->socket ); socket_close_complete( s->socket, ret ); @@ -4132,7 +4140,7 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas s->socket = socket; addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->recv_q, task_socket_close, s ))) + if ((ret = queue_task( &socket->recv_q, task_socket_close, &s->task_hdr ))) { release_object( &socket->hdr ); free( s ); diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 19ef9a35e2d..96bd7b53747 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -273,8 +273,16 @@ struct socket BOOL last_receive_final; }; +typedef void (*TASK_CALLBACK)( void *ctx ); + +struct task_header +{ + TASK_CALLBACK callback; +}; + struct send_request { + struct task_header task_hdr; struct request *request; WCHAR *headers; DWORD headers_len; @@ -286,17 +294,20 @@ struct send_request struct receive_response { + struct task_header task_hdr; struct request *request; }; struct query_data { + struct task_header task_hdr; struct request *request; DWORD *available; }; struct read_data { + struct task_header task_hdr; struct request *request; void *buffer; DWORD to_read; @@ -305,6 +316,7 @@ struct read_data struct write_data { + struct task_header task_hdr; struct request *request; const void *buffer; DWORD to_write; @@ -313,6 +325,7 @@ struct write_data struct socket_send { + struct task_header task_hdr; struct socket *socket; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; const void *buf; @@ -323,6 +336,7 @@ struct socket_send struct socket_receive { + struct task_header task_hdr; struct socket *socket; void *buf; DWORD len; @@ -330,6 +344,7 @@ struct socket_receive struct socket_shutdown { + struct task_header task_hdr; struct socket *socket; USHORT status; char reason[123]; -- 2.35.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 188 +++++++++++---------------------- dlls/winhttp/winhttp_private.h | 9 +- 2 files changed, 62 insertions(+), 135 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 36a3bb909b9..f98f5f30c2a 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -151,18 +151,27 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, T struct task_header *task_hdr = ctx; task_hdr->callback( task_hdr ); + release_object( task_hdr->obj ); + free( task_hdr ); } -static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr ) +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, + struct object_header *obj ) { TP_WORK *work; DWORD ret; if ((ret = start_queue( queue ))) return ret; - if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError(); + if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) + { + free( task_hdr ); + return GetLastError(); + } TRACE("queueing %p in %p\n", task_hdr, queue); task_hdr->callback = task; + task_hdr->obj = obj; + addref_object( obj ); SubmitThreadpoolWork( work ); CloseThreadpoolWork( work ); @@ -2178,13 +2187,12 @@ end: static void task_send_request( void *ctx ) { struct send_request *s = ctx; + struct request *request = (struct request *)s->task_hdr.obj; TRACE( "running %p\n", ctx ); - send_request( s->request, s->headers, s->headers_len, s->optional, s->optional_len, s->total_len, s->context, TRUE ); + send_request( request, s->headers, s->headers_len, s->optional, s->optional_len, s->total_len, s->context, TRUE ); - release_object( &s->request->hdr ); free( s->headers ); - free( s ); } /*********************************************************************** @@ -2216,23 +2224,19 @@ BOOL WINAPI WinHttpSendRequest( HINTERNET hrequest, const WCHAR *headers, DWORD if (request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) { struct send_request *s; + WCHAR *headers_dup; if (!(s = malloc( sizeof(*s) ))) return FALSE; - s->request = request; - s->headers = strdupW( headers ); + headers_dup = strdupW( headers ); + s->headers = headers_dup; s->headers_len = headers_len; s->optional = optional; s->optional_len = optional_len; s->total_len = total_len; s->context = context; - addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_send_request, &s->task_hdr ))) - { - release_object( &request->hdr ); - free( s->headers ); - free( s ); - } + if ((ret = queue_task( &request->queue, task_send_request, &s->task_hdr, &request->hdr ))) + free( headers_dup ); } else ret = send_request( request, headers, headers_len, optional, optional_len, total_len, context, FALSE ); @@ -2764,12 +2768,10 @@ static DWORD receive_response( struct request *request, BOOL async ) static void task_receive_response( void *ctx ) { struct receive_response *r = ctx; + struct request *request = (struct request *)r->task_hdr.obj; TRACE("running %p\n", ctx); - receive_response( r->request, TRUE ); - - release_object( &r->request->hdr ); - free( r ); + receive_response( request, TRUE ); } /*********************************************************************** @@ -2799,14 +2801,8 @@ BOOL WINAPI WinHttpReceiveResponse( HINTERNET hrequest, LPVOID reserved ) struct receive_response *r; if (!(r = malloc( sizeof(*r) ))) return FALSE; - r->request = request; - addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_receive_response, &r->task_hdr ))) - { - release_object( &request->hdr ); - free( r ); - } + ret = queue_task( &request->queue, task_receive_response, &r->task_hdr, &request->hdr ); } else ret = receive_response( request, FALSE ); @@ -2863,12 +2859,10 @@ done: static void task_query_data_available( void *ctx ) { struct query_data *q = ctx; + struct request *request = (struct request *)q->task_hdr.obj; TRACE("running %p\n", ctx); - query_data_available( q->request, q->available, TRUE ); - - release_object( &q->request->hdr ); - free( q ); + query_data_available( request, q->available, TRUE ); } /*********************************************************************** @@ -2899,16 +2893,10 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) struct query_data *q; if (!(q = malloc( sizeof(*q) ))) return FALSE; - q->request = request; q->available = available; - addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_query_data_available, &q->task_hdr ))) - { - release_object( &request->hdr ); - free( q ); - } - else ret = ERROR_IO_PENDING; + if (!(ret = queue_task( &request->queue, task_query_data_available, &q->task_hdr, &request->hdr ))) + ret = ERROR_IO_PENDING; } else ret = query_data_available( request, available, async ); @@ -2920,12 +2908,10 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) static void task_read_data( void *ctx ) { struct read_data *r = ctx; + struct request *request = (struct request *)r->task_hdr.obj; TRACE("running %p\n", ctx); - read_data( r->request, r->buffer, r->to_read, r->read, TRUE ); - - release_object( &r->request->hdr ); - free( r ); + read_data( request, r->buffer, r->to_read, r->read, TRUE ); } /*********************************************************************** @@ -2956,18 +2942,12 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW struct read_data *r; if (!(r = malloc( sizeof(*r) ))) return FALSE; - r->request = request; r->buffer = buffer; r->to_read = to_read; r->read = read; - addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_read_data, &r->task_hdr ))) - { - release_object( &request->hdr ); - free( r ); - } - else ret = ERROR_IO_PENDING; + if (!(ret = queue_task( &request->queue, task_read_data, &r->task_hdr, &request->hdr ))) + ret = ERROR_IO_PENDING; } else ret = read_data( request, buffer, to_read, read, async ); @@ -3001,12 +2981,10 @@ static DWORD write_data( struct request *request, const void *buffer, DWORD to_w static void task_write_data( void *ctx ) { struct write_data *w = ctx; + struct request *request = (struct request *)w->task_hdr.obj; TRACE("running %p\n", ctx); - write_data( w->request, w->buffer, w->to_write, w->written, TRUE ); - - release_object( &w->request->hdr ); - free( w ); + write_data( request, w->buffer, w->to_write, w->written, TRUE ); } /*********************************************************************** @@ -3036,13 +3014,11 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w struct write_data *w; if (!(w = malloc( sizeof(*w) ))) return FALSE; - w->request = request; w->buffer = buffer; w->to_write = to_write; w->written = written; - addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr ))) + if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr, &request->hdr ))) { release_object( &request->hdr ); free( w ); @@ -3403,19 +3379,17 @@ static DWORD socket_send( struct socket *socket, WINHTTP_WEB_SOCKET_BUFFER_TYPE static void task_socket_send( void *ctx ) { struct socket_send *s = ctx; + struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; TRACE("running %p\n", ctx); - if (s->complete_async) ret = complete_send_frame( s->socket, &s->ovr, s->buf ); - else ret = socket_send( s->socket, s->type, s->buf, s->len, NULL ); - - send_io_complete( &s->socket->hdr ); - InterlockedExchange( &s->socket->pending_noncontrol_send, 0 ); - socket_send_complete( s->socket, ret, s->type, s->len ); + if (s->complete_async) ret = complete_send_frame( socket, &s->ovr, s->buf ); + else ret = socket_send( socket, s->type, s->buf, s->len, NULL ); - release_object( &s->socket->hdr ); - free( s ); + send_io_complete( &socket->hdr ); + InterlockedExchange( &socket->pending_noncontrol_send, 0 ); + socket_send_complete( socket, ret, s->type, s->len ); } DWORD WINAPI WinHttpWebSocketSend( HINTERNET hsocket, WINHTTP_WEB_SOCKET_BUFFER_TYPE type, void *buf, DWORD len ) @@ -3481,21 +3455,13 @@ DWORD WINAPI WinHttpWebSocketSend( HINTERNET hsocket, WINHTTP_WEB_SOCKET_BUFFER_ { s->complete_async = complete_async; TRACE("queueing, complete_async %#x.\n", complete_async); - s->socket = socket; s->type = type; s->buf = buf; s->len = len; - addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_send, &s->task_hdr ))) - { - InterlockedDecrement( &socket->hdr.pending_sends ); - InterlockedExchange( &socket->pending_noncontrol_send, 0 ); - release_object( &socket->hdr ); - free( s ); - } + ret = queue_task( &socket->send_q, task_socket_send, &s->task_hdr, &socket->hdr ); } - else + if (!async_send || ret) { InterlockedDecrement( &socket->hdr.pending_sends ); InterlockedExchange( &socket->pending_noncontrol_send, 0 ); @@ -3607,16 +3573,14 @@ static DWORD receive_frame( struct socket *socket, DWORD *ret_len, enum socket_o static void task_socket_send_pong( void *ctx ) { struct socket_send *s = ctx; + struct socket *socket = (struct socket *)s->task_hdr.obj; TRACE("running %p\n", ctx); - if (s->complete_async) complete_send_frame( s->socket, &s->ovr, NULL ); - else send_frame( s->socket, SOCKET_OPCODE_PONG, 0, NULL, 0, TRUE, NULL ); - - send_io_complete( &s->socket->hdr ); + if (s->complete_async) complete_send_frame( socket, &s->ovr, NULL ); + else send_frame( socket, SOCKET_OPCODE_PONG, 0, NULL, 0, TRUE, NULL ); - release_object( &s->socket->hdr ); - free( s ); + send_io_complete( &socket->hdr ); } static DWORD socket_send_pong( struct socket *socket ) @@ -3644,14 +3608,8 @@ static DWORD socket_send_pong( struct socket *socket ) if (async_send) { s->complete_async = complete_async; - s->socket = socket; - addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_send_pong, &s->task_hdr ))) - { + if ((ret = queue_task( &socket->send_q, task_socket_send_pong, &s->task_hdr, &socket->hdr ))) InterlockedDecrement( &socket->hdr.pending_sends ); - release_object( &socket->hdr ); - free( s ); - } } else { @@ -3828,20 +3786,21 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD static void task_socket_receive( void *ctx ) { struct socket_receive *r = ctx; + struct socket *socket = (struct socket *)r->task_hdr.obj; DWORD ret, count; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; TRACE("running %p\n", ctx); - ret = socket_receive( r->socket, r->buf, r->len, &count, &type ); + ret = socket_receive( socket, r->buf, r->len, &count, &type ); - if (receive_io_complete( r->socket )) + if (receive_io_complete( socket )) { if (!ret) { WINHTTP_WEB_SOCKET_STATUS status; status.dwBytesTransferred = count; status.eBufferType = type; - send_callback( &r->socket->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, &status, sizeof(status) ); + send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_READ_COMPLETE, &status, sizeof(status) ); } else { @@ -3849,12 +3808,9 @@ static void task_socket_receive( void *ctx ) result.AsyncResult.dwResult = API_READ_DATA; result.AsyncResult.dwError = ret; result.Operation = WINHTTP_WEB_SOCKET_RECEIVE_OPERATION; - send_callback( &r->socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); + send_callback( &socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); } } - - release_object( &r->socket->hdr ); - free( r ); } DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, DWORD *ret_len, @@ -3896,17 +3852,11 @@ DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, D InterlockedDecrement( &socket->hdr.pending_receives ); return ERROR_OUTOFMEMORY; } - r->socket = socket; r->buf = buf; r->len = len; - addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->recv_q, task_socket_receive, &r->task_hdr ))) - { + if ((ret = queue_task( &socket->recv_q, task_socket_receive, &r->task_hdr, &socket->hdr ))) InterlockedDecrement( &socket->hdr.pending_receives ); - release_object( &socket->hdr ); - free( r ); - } } else ret = socket_receive( socket, buf, len, ret_len, ret_type ); @@ -3930,17 +3880,16 @@ static void socket_shutdown_complete( struct socket *socket, DWORD ret ) static void task_socket_shutdown( void *ctx ) { struct socket_shutdown *s = ctx; + struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; TRACE("running %p\n", ctx); - if (s->complete_async) ret = complete_send_frame( s->socket, &s->ovr, s->reason ); - else ret = send_frame( s->socket, SOCKET_OPCODE_CLOSE, s->status, s->reason, s->len, TRUE, NULL ); + if (s->complete_async) ret = complete_send_frame( socket, &s->ovr, s->reason ); + else ret = send_frame( socket, SOCKET_OPCODE_CLOSE, s->status, s->reason, s->len, TRUE, NULL ); - send_io_complete( &s->socket->hdr ); - if (s->send_callback) socket_shutdown_complete( s->socket, ret ); - release_object( &s->socket->hdr ); - free( s ); + send_io_complete( &socket->hdr ); + if (s->send_callback) socket_shutdown_complete( socket, ret ); } static DWORD send_socket_shutdown( struct socket *socket, USHORT status, const void *reason, DWORD len, @@ -3972,19 +3921,13 @@ static DWORD send_socket_shutdown( struct socket *socket, USHORT status, const v if (async_send) { s->complete_async = complete_async; - s->socket = socket; s->status = status; memcpy( s->reason, reason, len ); s->len = len; s->send_callback = send_callback; - addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->send_q, task_socket_shutdown, &s->task_hdr ))) - { + if ((ret = queue_task( &socket->send_q, task_socket_shutdown, &s->task_hdr, &socket->hdr ))) InterlockedDecrement( &socket->hdr.pending_sends ); - release_object( &socket->hdr ); - free( s ); - } } else InterlockedDecrement( &socket->hdr.pending_sends ); ReleaseSRWLockExclusive( &socket->send_lock ); @@ -4068,15 +4011,13 @@ static void socket_close_complete( struct socket *socket, DWORD ret ) static void task_socket_close( void *ctx ) { struct socket_shutdown *s = ctx; + struct socket *socket = (struct socket *)s->task_hdr.obj; DWORD ret; TRACE("running %p\n", ctx); - ret = socket_close( s->socket ); - socket_close_complete( s->socket, ret ); - - release_object( &s->socket->hdr ); - free( s ); + ret = socket_close( socket ); + socket_close_complete( socket, ret ); } DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reason, DWORD len ) @@ -4137,14 +4078,7 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas struct socket_shutdown *s; if (!(s = calloc( 1, sizeof(*s) ))) return FALSE; - s->socket = socket; - - addref_object( &socket->hdr ); - if ((ret = queue_task( &socket->recv_q, task_socket_close, &s->task_hdr ))) - { - release_object( &socket->hdr ); - free( s ); - } + ret = queue_task( &socket->recv_q, task_socket_close, &s->task_hdr, &socket->hdr ); } else ret = socket_close( socket ); diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 96bd7b53747..35f3e64b5a3 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -278,12 +278,12 @@ typedef void (*TASK_CALLBACK)( void *ctx ); struct task_header { TASK_CALLBACK callback; + struct object_header *obj; }; struct send_request { struct task_header task_hdr; - struct request *request; WCHAR *headers; DWORD headers_len; void *optional; @@ -295,20 +295,17 @@ struct send_request struct receive_response { struct task_header task_hdr; - struct request *request; }; struct query_data { struct task_header task_hdr; - struct request *request; DWORD *available; }; struct read_data { struct task_header task_hdr; - struct request *request; void *buffer; DWORD to_read; DWORD *read; @@ -317,7 +314,6 @@ struct read_data struct write_data { struct task_header task_hdr; - struct request *request; const void *buffer; DWORD to_write; DWORD *written; @@ -326,7 +322,6 @@ struct write_data struct socket_send { struct task_header task_hdr; - struct socket *socket; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; const void *buf; DWORD len; @@ -337,7 +332,6 @@ struct socket_send struct socket_receive { struct task_header task_hdr; - struct socket *socket; void *buf; DWORD len; }; @@ -345,7 +339,6 @@ struct socket_receive struct socket_shutdown { struct task_header task_hdr; - struct socket *socket; USHORT status; char reason[123]; DWORD len; -- 2.35.1
On Mon, 2022-03-14 at 20:04 +0300, Paul Gofman wrote:
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 36a3bb909b9..f98f5f30c2a 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -151,18 +151,27 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, T struct task_header *task_hdr = ctx; task_hdr->callback( task_hdr ); + release_object( task_hdr->obj ); + free( task_hdr ); } -static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr ) +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, + struct object_header *obj ) { TP_WORK *work; DWORD ret; if ((ret = start_queue( queue ))) return ret; - if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError(); + if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) + { + free( task_hdr ); + return GetLastError(); + }
You missed freeing task_hdr when start_queue() fails, though I'd prefer freeing it in the callers. Then you also wouldn't need that headers_dup variable in WinHttpSendRequest().
@@ -3036,13 +3014,11 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w struct write_data *w;
if (!(w = malloc( sizeof(*w) ))) return FALSE; - w->request = request; w->buffer = buffer; w->to_write = to_write; w->written = written;
- addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr))) + if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr, &request->hdr ))) { release_object( &request->hdr ); free( w );
This release_object() call should also be removed.
On 3/15/22 18:26, Hans Leidekker wrote:
On Mon, 2022-03-14 at 20:04 +0300, Paul Gofman wrote:
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 36a3bb909b9..f98f5f30c2a 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -151,18 +151,27 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, T struct task_header *task_hdr = ctx;
task_hdr->callback( task_hdr ); + release_object( task_hdr->obj ); + free( task_hdr ); }
-static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr ) +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, + struct object_header *obj ) { TP_WORK *work; DWORD ret;
if ((ret = start_queue( queue ))) return ret;
- if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError(); + if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) + { + free( task_hdr ); + return GetLastError(); + } You missed freeing task_hdr when start_queue() fails, though I'd prefer freeing it in the callers. Then you also wouldn't need that headers_dup variable in WinHttpSendRequest().
Do you mean always leaving the free on queue_task() failure where it is before the patch?
@@ -3036,13 +3014,11 @@ BOOL WINAPI WinHttpWriteData( HINTERNET hrequest, const void *buffer, DWORD to_w struct write_data *w;
if (!(w = malloc( sizeof(*w) ))) return FALSE; - w->request = request; w->buffer = buffer; w->to_write = to_write; w->written = written;
- addref_object( &request->hdr ); - if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr))) + if ((ret = queue_task( &request->queue, task_write_data, &w->task_hdr, &request->hdr ))) { release_object( &request->hdr ); free( w ); This release_object() call should also be removed.
Indeed, thanks. You signed off the last patch, do you mean I can just resend this one?
On 3/15/22 18:26, Hans Leidekker wrote:
On Mon, 2022-03-14 at 20:04 +0300, Paul Gofman wrote:
diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 36a3bb909b9..f98f5f30c2a 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -151,18 +151,27 @@ static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, T struct task_header *task_hdr = ctx;
task_hdr->callback( task_hdr ); + release_object( task_hdr->obj ); + free( task_hdr ); }
-static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr ) +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, + struct object_header *obj ) { TP_WORK *work; DWORD ret;
if ((ret = start_queue( queue ))) return ret;
- if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) return GetLastError(); + if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) + { + free( task_hdr ); + return GetLastError(); + } You missed freeing task_hdr when start_queue() fails, though I'd prefer freeing it in the callers. Then you also wouldn't need that headers_dup variable in WinHttpSendRequest().
Do you mean always leaving the free on queue_task() failure where it is before the patch?
Yes.
You signed off the last patch, do you mean I can just resend this one?
Yes, it looks good.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 88 ++++++++++++++++++++++------------ dlls/winhttp/session.c | 1 + dlls/winhttp/winhttp_private.h | 7 ++- 3 files changed, 63 insertions(+), 33 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index f98f5f30c2a..56cfe218401 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -122,58 +122,82 @@ static const WCHAR *attribute_table[] = NULL /* WINHTTP_QUERY_PASSPORT_CONFIG = 78 */ }; -static DWORD start_queue( struct queue *queue ) +void init_queue( struct queue *queue ) { - if (queue->pool) return ERROR_SUCCESS; - - if (!(queue->pool = CreateThreadpool( NULL ))) return GetLastError(); - SetThreadpoolThreadMinimum( queue->pool, 1 ); - SetThreadpoolThreadMaximum( queue->pool, 1 ); - - memset( &queue->env, 0, sizeof(queue->env) ); - queue->env.Version = 1; - queue->env.Pool = queue->pool; - - TRACE("started %p\n", queue); - return ERROR_SUCCESS; + InitializeSRWLock( &queue->lock ); + list_init( &queue->queued_tasks ); + queue->callback_running = FALSE; } void stop_queue( struct queue *queue ) { - if (!queue->pool) return; - CloseThreadpool( queue->pool ); - queue->pool = NULL; + assert( list_empty( &queue->queued_tasks )); TRACE("stopped %p\n", queue); } -static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) +static struct task_header *get_next_task( struct queue *queue ) { - struct task_header *task_hdr = ctx; + struct list *entry; - task_hdr->callback( task_hdr ); - release_object( task_hdr->obj ); - free( task_hdr ); + AcquireSRWLockExclusive( &queue->lock ); + assert( queue->callback_running ); + if ((entry = list_head( &queue->queued_tasks ))) + list_remove( entry ); + else + queue->callback_running = FALSE; + ReleaseSRWLockExclusive( &queue->lock ); + if (!entry) return NULL; + return LIST_ENTRY( entry, struct task_header, entry ); } -static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, - struct object_header *obj ) +static void CALLBACK task_callback( TP_CALLBACK_INSTANCE *instance, void *ctx ) { - TP_WORK *work; - DWORD ret; + struct task_header *task, *next_task; + struct queue *queue = ctx; - if ((ret = start_queue( queue ))) return ret; + TRACE( "instance %p.\n", instance ); - if (!(work = CreateThreadpoolWork( task_callback, task_hdr, &queue->env ))) + task = get_next_task( queue ); + while (task) { - free( task_hdr ); - return GetLastError(); + task->callback( task ); + /* Queue object may be freed by release_object() unless there is another task referencing it. */ + next_task = get_next_task( queue ); + release_object( task->obj ); + free( task ); + task = next_task; } + TRACE( "instance %p exiting.\n", instance ); +} + +static DWORD queue_task( struct queue *queue, TASK_CALLBACK task, struct task_header *task_hdr, + struct object_header *obj ) +{ + BOOL callback_running; + TRACE("queueing %p in %p\n", task_hdr, queue); task_hdr->callback = task; task_hdr->obj = obj; addref_object( obj ); - SubmitThreadpoolWork( work ); - CloseThreadpoolWork( work ); + + AcquireSRWLockExclusive( &queue->lock ); + list_add_tail( &queue->queued_tasks, &task_hdr->entry ); + if (!(callback_running = queue->callback_running)) + { + if ((queue->callback_running = TrySubmitThreadpoolCallback( task_callback, queue, NULL ))) + callback_running = TRUE; + else + list_remove( &task_hdr->entry ); + } + ReleaseSRWLockExclusive( &queue->lock ); + + if (!callback_running) + { + release_object( obj ); + free( task_hdr ); + ERR( "Submiting threadpool callback failed, err %lu.\n", GetLastError() ); + return ERROR_OUTOFMEMORY; + } return ERROR_SUCCESS; } @@ -3097,6 +3121,8 @@ HINTERNET WINAPI WinHttpWebSocketCompleteUpgrade( HINTERNET hrequest, DWORD_PTR socket->hdr.notify_mask = request->hdr.notify_mask; socket->hdr.context = context; InitializeSRWLock( &socket->send_lock ); + init_queue( &socket->send_q ); + init_queue( &socket->recv_q ); addref_object( &request->hdr ); socket->request = request; diff --git a/dlls/winhttp/session.c b/dlls/winhttp/session.c index 7c55adb8475..3b00d2448d1 100644 --- a/dlls/winhttp/session.c +++ b/dlls/winhttp/session.c @@ -1160,6 +1160,7 @@ HINTERNET WINAPI WinHttpOpenRequest( HINTERNET hconnect, const WCHAR *verb, cons request->hdr.notify_mask = connect->hdr.notify_mask; request->hdr.context = connect->hdr.context; request->hdr.redirect_policy = connect->hdr.redirect_policy; + init_queue( &request->queue ); addref_object( &connect->hdr ); request->connect = connect; diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 35f3e64b5a3..4754d9258d1 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -159,8 +159,9 @@ struct authinfo struct queue { - TP_POOL *pool; - TP_CALLBACK_ENVIRON env; + SRWLOCK lock; + struct list queued_tasks; + BOOL callback_running; }; enum request_flags @@ -277,6 +278,7 @@ typedef void (*TASK_CALLBACK)( void *ctx ); struct task_header { + struct list entry; TASK_CALLBACK callback; struct object_header *obj; }; @@ -355,6 +357,7 @@ BOOL free_handle( HINTERNET ) DECLSPEC_HIDDEN; void send_callback( struct object_header *, DWORD, LPVOID, DWORD ) DECLSPEC_HIDDEN; void close_connection( struct request * ) DECLSPEC_HIDDEN; +void init_queue( struct queue *queue ) DECLSPEC_HIDDEN; void stop_queue( struct queue * ) DECLSPEC_HIDDEN; void netconn_close( struct netconn * ) DECLSPEC_HIDDEN; -- 2.35.1
Signed-off-by: Hans Leidekker <hans(a)codeweavers.com>
participants (2)
-
Hans Leidekker -
Paul Gofman