[PATCH v2 1/6] winhttp: Move receive callback call to task_socket_receive().
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes. dlls/winhttp/request.c | 48 +++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index f37c33b202c..bddbf4a03b2 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3578,7 +3578,7 @@ static WINHTTP_WEB_SOCKET_BUFFER_TYPE map_opcode( enum socket_opcode opcode, BOO } static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD *ret_len, - WINHTTP_WEB_SOCKET_BUFFER_TYPE *ret_type, BOOL async ) + WINHTTP_WEB_SOCKET_BUFFER_TYPE *ret_type ) { DWORD count, ret = ERROR_SUCCESS; @@ -3601,29 +3601,8 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD WARN("Short read.\n"); socket->read_size -= count; - if (!async) - { - *ret_len = count; - *ret_type = map_opcode( socket->opcode, socket->read_size != 0 ); - } - } - if (async) - { - if (!ret) - { - WINHTTP_WEB_SOCKET_STATUS status; - status.dwBytesTransferred = count; - status.eBufferType = map_opcode( socket->opcode, socket->read_size != 0 ); - 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) ); - } + *ret_len = count; + *ret_type = map_opcode( socket->opcode, socket->read_size != 0 ); } return ret; } @@ -3631,9 +3610,26 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD static void CALLBACK task_socket_receive( TP_CALLBACK_INSTANCE *instance, void *ctx, TP_WORK *work ) { struct socket_receive *r = ctx; + DWORD ret, count, type; TRACE("running %p\n", work); - socket_receive( r->socket, r->buf, r->len, NULL, NULL, TRUE ); + ret = socket_receive( r->socket, r->buf, r->len, &count, &type ); + + 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) ); + } + 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( &r->socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); + } release_object( &r->socket->hdr ); free( r ); @@ -3677,7 +3673,7 @@ DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, D free( r ); } } - else ret = socket_receive( socket, buf, len, ret_len, ret_type, FALSE ); + else ret = socket_receive( socket, buf, len, ret_len, ret_type ); release_object( &socket->hdr ); return ret; -- 2.34.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes. dlls/winhttp/request.c | 9 +++++++++ dlls/winhttp/winhttp_private.h | 1 + 2 files changed, 10 insertions(+) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index bddbf4a03b2..7666bae4e68 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3244,6 +3244,12 @@ static void send_io_complete( struct object_header *hdr ) assert( count >= 0 ); } +static void receive_io_complete( struct socket *socket ) +{ + LONG count = InterlockedDecrement( &socket->hdr.pending_receives ); + assert( count >= 0 ); +} + static enum socket_opcode map_buffer_type( WINHTTP_WEB_SOCKET_BUFFER_TYPE type ) { switch (type) @@ -3614,6 +3620,7 @@ static void CALLBACK task_socket_receive( TP_CALLBACK_INSTANCE *instance, void * TRACE("running %p\n", work); ret = socket_receive( r->socket, r->buf, r->len, &count, &type ); + receive_io_complete( r->socket ); if (!ret) { @@ -3667,8 +3674,10 @@ DWORD WINAPI WinHttpWebSocketReceive( HINTERNET hsocket, void *buf, DWORD len, D r->len = len; addref_object( &socket->hdr ); + InterlockedIncrement( &socket->hdr.pending_receives ); if ((ret = queue_task( &socket->recv_q, task_socket_receive, r ))) { + InterlockedDecrement( &socket->hdr.pending_receives ); release_object( &socket->hdr ); free( r ); } diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 5eb3e97a916..a6638aba30f 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -52,6 +52,7 @@ struct object_header LONG recursion_count; struct list entry; volatile LONG pending_sends; + volatile LONG pending_receives; }; struct hostdata -- 2.34.1
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - restore check for socket state before sending shutdown. dlls/winhttp/request.c | 65 ++++++++++++++++++++++--------- dlls/winhttp/tests/notification.c | 18 ++++++++- 2 files changed, 63 insertions(+), 20 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 7666bae4e68..39223d41bbb 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3244,10 +3244,14 @@ static void send_io_complete( struct object_header *hdr ) assert( count >= 0 ); } -static void receive_io_complete( struct socket *socket ) +/* returns FALSE if sending callback should be omitted. */ +static BOOL receive_io_complete( struct socket *socket ) { LONG count = InterlockedDecrement( &socket->hdr.pending_receives ); - assert( count >= 0 ); + 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; } static enum socket_opcode map_buffer_type( WINHTTP_WEB_SOCKET_BUFFER_TYPE type ) @@ -3620,22 +3624,24 @@ static void CALLBACK task_socket_receive( TP_CALLBACK_INSTANCE *instance, void * TRACE("running %p\n", work); ret = socket_receive( r->socket, r->buf, r->len, &count, &type ); - receive_io_complete( r->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) ); - } - else + if (receive_io_complete( r->socket )) { - 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( &r->socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); + 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) ); + } + 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( &r->socket->hdr, WINHTTP_CALLBACK_STATUS_REQUEST_ERROR, &result, sizeof(result) ); + } } release_object( &r->socket->hdr ); @@ -3719,7 +3725,7 @@ static DWORD send_socket_shutdown( struct socket *socket, USHORT status, const v { DWORD ret; - socket->state = SOCKET_STATE_SHUTDOWN; + if (socket->state < SOCKET_STATE_SHUTDOWN) socket->state = SOCKET_STATE_SHUTDOWN; if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) { @@ -3816,6 +3822,8 @@ static void CALLBACK task_socket_close( TP_CALLBACK_INSTANCE *instance, void *ct DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reason, DWORD len ) { + enum socket_state prev_state; + LONG pending_receives = 0; struct socket *socket; DWORD ret; @@ -3835,10 +3843,29 @@ DWORD WINAPI WinHttpWebSocketClose( HINTERNET hsocket, USHORT status, void *reas return ERROR_INVALID_OPERATION; } - if (socket->state < SOCKET_STATE_SHUTDOWN + prev_state = socket->state; + socket->state = SOCKET_STATE_CLOSED; + + 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) ); + } + } + + if (prev_state < SOCKET_STATE_SHUTDOWN && (ret = send_socket_shutdown( socket, status, reason, len, FALSE ))) goto done; - socket->state = SOCKET_STATE_CLOSED; if (socket->request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) { struct socket_shutdown *s; diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 2707151e00f..2db5a094f87 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -63,6 +63,7 @@ struct notification #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 */ struct info { @@ -75,6 +76,8 @@ struct info DWORD main_thread_id; DWORD last_thread_id; DWORD last_status; + char buffer[256]; + unsigned int buflen; }; struct test_request @@ -118,6 +121,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_SAVE_BUFFER) + { + info->buflen = buflen; + memcpy( info->buffer, buffer, min( buflen, sizeof(info->buffer) )); + } if (status_ok && function_ok && info->test[info->index++].flags & NF_SIGNAL) { @@ -694,7 +702,7 @@ static const struct notification websocket_test2[] = { 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 }, + { 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_close_handle, WINHTTP_CALLBACK_STATUS_CLOSING_CONNECTION, NF_WINE_ALLOW }, @@ -707,6 +715,7 @@ static const struct notification websocket_test2[] = static void test_websocket(BOOL secure) { HANDLE session, connection, request, socket, event; + WINHTTP_WEB_SOCKET_ASYNC_RESULT *result; WINHTTP_WEB_SOCKET_BUFFER_TYPE type; DWORD size, status, err; BOOL ret, unload = TRUE; @@ -951,6 +960,13 @@ static void test_websocket(BOOL secure) setup_test( &info, winhttp_websocket_close, __LINE__ ); ret = pWinHttpWebSocketClose( socket, 1000, (void *)"success", sizeof("success") ); ok( err == ERROR_SUCCESS, "got %u\n", err ); + ok( info.buflen == sizeof(*result), "got unexpected buflen %u.\n", info.buflen ); + result = (WINHTTP_WEB_SOCKET_ASYNC_RESULT *)info.buffer; + ok( result->Operation == WINHTTP_WEB_SOCKET_RECEIVE_OPERATION, "got unexpected operation %u.\n", + result->Operation ); + ok( !result->AsyncResult.dwResult, "got unexpected result %u.\n", result->AsyncResult.dwResult ); + ok( result->AsyncResult.dwError == ERROR_WINHTTP_OPERATION_CANCELLED, "got unexpected error %u.\n", + result->AsyncResult.dwError ); close_status = 0xdead; size = sizeof(buffer) + 1; -- 2.34.1
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=106073 Your paranoid android. === w8adm (32 bit report) === winhttp: notification.c:256: Test failed: failed to send request 12007 notification.c:258: Test failed: unexpected function 3, expected 4. probably some notifications were missing notification.c:260: Test failed: failed to receive response 12019 notification.c:264: Test failed: failed unexpectedly 12019 notification.c:265: Test failed: request failed unexpectedly 0 notification.c:268: Test failed: unexpected function 3, expected 13. probably some notifications were missing notification.c:116: Test failed: 268: expected status 0x00000002 got 0x00000800 notification.c:117: Test failed: 268: expected function 3 got 13 notification: Timeout
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes. dlls/winhttp/request.c | 6 ++++++ dlls/winhttp/tests/notification.c | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 39223d41bbb..279e0522166 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3909,6 +3909,12 @@ DWORD WINAPI WinHttpWebSocketQueryCloseStatus( HINTERNET hsocket, USHORT *status return ERROR_INVALID_OPERATION; } + if (!socket->close_frame_received || socket->close_frame_receive_err) + { + ret = socket->close_frame_received ? socket->close_frame_receive_err : ERROR_INVALID_OPERATION; + release_object( &socket->hdr ); + return ret; + } *status = socket->status; *ret_len = socket->reason_len; if (socket->reason_len > len) ret = ERROR_INSUFFICIENT_BUFFER; diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 2db5a094f87..1abc8cf377f 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -971,9 +971,9 @@ static void test_websocket(BOOL secure) close_status = 0xdead; size = sizeof(buffer) + 1; err = pWinHttpWebSocketQueryCloseStatus( socket, &close_status, buffer, sizeof(buffer), &size ); - todo_wine ok( err == ERROR_INVALID_OPERATION, "got %u\n", err ); - todo_wine ok( close_status == 0xdead, "got %u\n", close_status ); - todo_wine ok( size == sizeof(buffer) + 1, "got %u\n", size ); + ok( err == ERROR_INVALID_OPERATION, "got %u\n", err ); + ok( close_status == 0xdead, "got %u\n", close_status ); + ok( size == sizeof(buffer) + 1, "got %u\n", size ); WaitForSingleObject( info.wait, INFINITE ); -- 2.34.1
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=106074 Your paranoid android. === w8adm (32 bit report) === winhttp: notification.c:256: Test failed: failed to send request 12007 notification.c:258: Test failed: unexpected function 3, expected 4. probably some notifications were missing notification.c:260: Test failed: failed to receive response 12019 notification.c:264: Test failed: failed unexpectedly 12019 notification.c:265: Test failed: request failed unexpectedly 0 notification.c:268: Test failed: unexpected function 3, expected 13. probably some notifications were missing notification.c:116: Test failed: 268: expected status 0x00000002 got 0x00000800 notification.c:117: Test failed: 268: expected function 3 got 13 notification: Timeout
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes. As testing involves waiting for timeouts it doesn't look good for inclusion into the test suite. I've tested this change separately: https://gist.github.com/gofman/bd2f3b6fd6cbe993e97353560ce81e04 This test succeeds on Windows for me but fails without this patch in Wine: the server close status received during closing handshake becomes WINHTTP_WEB_SOCKET_PROTOCOL_ERROR_CLOSE_STATUS (1002). Sending ping instead of pong causes the server to send a pong reply as expected although I still see status 1002 on close after. Which probably suggests that Windows doesn't send the ping as well (I also tried that with WINHTTP_OPTION_WEB_SOCKET_KEEPALIVE_INTERVAL socket option set to 15000 and Sleep to 30000). Also, while looking at this I spotted that we currently ignore PING application data (which may be present according to rfc6455 and should be echoed back with PONG. But I didn't see any server sending pings yet and didn't do anything with this part so far. dlls/winhttp/request.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 279e0522166..41a4cef9bc3 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3600,7 +3600,11 @@ static DWORD socket_receive( struct socket *socket, void *buf, DWORD len, DWORD { if (!(socket->opcode & CONTROL_BIT) || (ret = handle_control_frame( socket ))) break; } - else if (ret == WSAETIMEDOUT) ret = socket_send_pong( socket ); + else if (ret == WSAETIMEDOUT) + { + WARN( "timeout, retrying.\n" ); + continue; + } if (ret) break; } } -- 2.34.1
On Wed, 2022-01-26 at 16:14 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes.
As testing involves waiting for timeouts it doesn't look good for inclusion into the test suite. I've tested this change separately: https://gist.github.com/gofman/bd2f3b6fd6cbe993e97353560ce81e04 This test succeeds on Windows for me but fails without this patch in Wine: the server close status received during closing handshake becomes WINHTTP_WEB_SOCKET_PROTOCOL_ERROR_CLOSE_STATUS (1002).
Sending ping instead of pong causes the server to send a pong reply as expected although I still see status 1002 on close after. Which probably suggests that Windows doesn't send the ping as well (I also tried that with WINHTTP_OPTION_WEB_SOCKET_KEEPALIVE_INTERVAL socket option set to 15000 and Sleep to 30000).
It should be an unsollicited pong, which serves as a keepalive packet. I'm pretty sure I observed this, although I don't remember the exact conditions.
On 1/26/22 17:26, Hans Leidekker wrote:
On Wed, 2022-01-26 at 16:14 +0300, Paul Gofman wrote:
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- v2: - no changes.
As testing involves waiting for timeouts it doesn't look good for inclusion into the test suite. I've tested this change separately: https://gist.github.com/gofman/bd2f3b6fd6cbe993e97353560ce81e04 This test succeeds on Windows for me but fails without this patch in Wine: the server close status received during closing handshake becomes WINHTTP_WEB_SOCKET_PROTOCOL_ERROR_CLOSE_STATUS (1002).
Sending ping instead of pong causes the server to send a pong reply as expected although I still see status 1002 on close after. Which probably suggests that Windows doesn't send the ping as well (I also tried that with WINHTTP_OPTION_WEB_SOCKET_KEEPALIVE_INTERVAL socket option set to 15000 and Sleep to 30000). It should be an unsollicited pong, which serves as a keepalive packet. I'm pretty sure I observed this, although I don't remember the exact conditions.
I did observer unsolicited pong from server with Halo Infinite servers. Although I am not sure if we should do anything with it (sending some app callback?) instead of ignoring.
Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- That is a leftover from the previous version of the earlier committed patches. I think it didn't introduce a regression but prevents further sync sends once a send was queued async. v2: - added patch. dlls/winhttp/request.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 41a4cef9bc3..ed1336925e4 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3375,7 +3375,6 @@ DWORD WINAPI WinHttpWebSocketSend( HINTERNET hsocket, WINHTTP_WEB_SOCKET_BUFFER_ release_object( &socket->hdr ); free( s ); } - else ++socket->hdr.pending_sends; } else { -- 2.34.1
participants (3)
-
Hans Leidekker -
Marvin -
Paul Gofman