[PATCH v3 0/2] MR4925: winhttp: Get output synchronously whenever possible in WinHttpQueryDataAvailable / WinHttpReadData.
It looks like when the data is available but the recursion limit is reached Windows (after 7 when WinHttpQueryDataAvailable is concerned) fills the output immediately and only queues completion callback. I hope the updated test should be stable as we should have the server response read during WinHttpSendRequest already (like on Windows). Fixes market window in Black Desert Online. The game depends on WinHttpReadData returning the result synchronously in WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE callback (and doesn't mind WINHTTP_CALLBACK_STATUS_READ_COMPLETE at all). By the time it reaches there through previous callbacks the recursion limit doesn't allow full synchronous return (and after playing with test_recursion() I think at least Windows 10 has the same recursion limit as we do currently). But as the test shows the output of WinHttpReadData / WinHttpQueryDataAvailable is filled in immediately, probably only the callback should be queued asynchronously to break the recursion. -- v3: winhttp: Always return result at once if available in WinHttpReadData(). winhttp: Always return result at once if available in WinHttpQueryDataAvailable(). https://gitlab.winehq.org/wine/wine/-/merge_requests/4925
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 59 ++++++++++++++++++++++++++++--- dlls/winhttp/tests/notification.c | 11 +++++- dlls/winhttp/winhttp_private.h | 13 +++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 84af8e58988..59431e3a9f2 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -257,6 +257,16 @@ static BOOL cancel_queue( struct queue *queue ) return cancelled; } +static void task_send_callback( void *ctx, BOOL abort ) +{ + struct send_callback *s = ctx; + + if (abort) return; + + TRACE( "running %p\n", ctx ); + send_callback( s->task_hdr.obj, s->status, s->info, s->buflen ); +} + static void free_header( struct header *header ) { free( header->field ); @@ -3052,9 +3062,10 @@ static DWORD query_data_ready( struct request *request ) return count; } -static BOOL skip_async_queue( struct request *request ) +static BOOL skip_async_queue( struct request *request, BOOL *wont_block ) { - return request->hdr.recursion_count < 3 && (end_of_read_data( request ) || query_data_ready( request )); + *wont_block = end_of_read_data( request ) || query_data_ready( request ); + return request->hdr.recursion_count < 3 && *wont_block; } static DWORD query_data_available( struct request *request, DWORD *available, BOOL async ) @@ -3106,6 +3117,7 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) DWORD ret; struct request *request; BOOL async; + BOOL wont_block = FALSE; TRACE("%p, %p\n", hrequest, available); @@ -3121,7 +3133,44 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) return FALSE; } - if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request )) + if (!(async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) || skip_async_queue( request, &wont_block )) + { + ret = query_data_available( request, available, async ); + } + else if (wont_block) + { + /* Data available but recursion limit reached, only queue callback. */ + struct send_callback *s; + + if (!(s = malloc( sizeof(*s) ))) + { + release_object( &request->hdr ); + SetLastError( ERROR_OUTOFMEMORY ); + return FALSE; + } + + if (!(ret = query_data_available( request, &s->count, FALSE ))) + { + if (available) *available = s->count; + s->status = WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE; + s->info = &s->count; + s->buflen = sizeof(s->count); + } + else + { + s->result.dwResult = API_QUERY_DATA_AVAILABLE; + s->result.dwError = ret; + s->status = WINHTTP_CALLBACK_STATUS_REQUEST_ERROR; + s->info = &s->result; + s->buflen = sizeof(s->result); + } + + if ((ret = queue_task( &request->queue, task_send_callback, &s->task_hdr, &request->hdr ))) + free( s ); + else + ret = ERROR_IO_PENDING; + } + else { struct query_data *q; @@ -3139,7 +3188,6 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available ) else ret = ERROR_IO_PENDING; } - else ret = query_data_available( request, available, async ); release_object( &request->hdr ); SetLastError( ret ); @@ -3165,6 +3213,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW DWORD ret; struct request *request; BOOL async; + BOOL wont_block; TRACE( "%p, %p, %lu, %p\n", hrequest, buffer, to_read, read ); @@ -3180,7 +3229,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW return FALSE; } - if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request )) + if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request, &wont_block )) { struct read_data *r; diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 78319f0df5c..98df1571464 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1929,6 +1929,9 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex break; case WINHTTP_CALLBACK_STATUS_READ_COMPLETE: + { + static DWORD len; + if (!buflen) { SetEvent( context->wait ); @@ -1939,13 +1942,19 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex context->max_recursion_read = max( context->max_recursion_read, context->recursion_count ); context->read_from_callback = TRUE; InterlockedIncrement( &context->recursion_count ); - ret = WinHttpQueryDataAvailable( context->request, NULL ); + len = 0xdeadbeef; + /* Use static variable len here so write to it doesn't destroy the stack on old Windows which + * doesn't set the value at once. */ + ret = WinHttpQueryDataAvailable( context->request, &len ); err = GetLastError(); ok( ret, "failed to query data available, GetLastError() %lu\n", err ); ok( err == ERROR_SUCCESS || err == ERROR_IO_PENDING, "got %lu\n", err ); + ok( len != 0xdeadbeef || broken( len == 0xdeadbeef ) /* Win7 */, "got %lu.\n", len ); if (err == ERROR_SUCCESS) context->have_sync_callback = TRUE; InterlockedDecrement( &context->recursion_count ); break; + } + case WINHTTP_CALLBACK_STATUS_RECEIVING_RESPONSE: if (!context->headers_available && context->call_receive_response_status == WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE) diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h index 0bf1c5bc6b0..80ce2614ca1 100644 --- a/dlls/winhttp/winhttp_private.h +++ b/dlls/winhttp/winhttp_private.h @@ -312,6 +312,19 @@ struct task_header volatile LONG completion_sent; }; +struct send_callback +{ + struct task_header task_hdr; + DWORD status; + void *info; + DWORD buflen; + union + { + WINHTTP_ASYNC_RESULT result; + DWORD count; + }; +}; + struct send_request { struct task_header task_hdr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4925
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winhttp/request.c | 42 ++++++++++++++++++++++++++++--- dlls/winhttp/tests/notification.c | 10 +++++++- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index 59431e3a9f2..29b30fc2583 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -3213,7 +3213,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW DWORD ret; struct request *request; BOOL async; - BOOL wont_block; + BOOL wont_block = FALSE; TRACE( "%p, %p, %lu, %p\n", hrequest, buffer, to_read, read ); @@ -3229,7 +3229,44 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW return FALSE; } - if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request, &wont_block )) + if (!(async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) || skip_async_queue( request, &wont_block )) + { + ret = read_data( request, buffer, to_read, read, async ); + } + else if (wont_block) + { + /* Data available but recursion limit reached, only queue callback. */ + struct send_callback *s; + + if (!(s = malloc( sizeof(*s) ))) + { + release_object( &request->hdr ); + SetLastError( ERROR_OUTOFMEMORY ); + return FALSE; + } + + if (!(ret = read_data( request, buffer, to_read, &s->count, FALSE ))) + { + if (read) *read = s->count; + s->status = WINHTTP_CALLBACK_STATUS_READ_COMPLETE; + s->info = buffer; + s->buflen = s->count; + } + else + { + s->result.dwResult = API_READ_DATA; + s->result.dwError = ret; + s->status = WINHTTP_CALLBACK_STATUS_REQUEST_ERROR; + s->info = &s->result; + s->buflen = sizeof(s->result); + } + + if ((ret = queue_task( &request->queue, task_send_callback, &s->task_hdr, &request->hdr ))) + free( s ); + else + ret = ERROR_IO_PENDING; + } + else { struct read_data *r; @@ -3248,7 +3285,6 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW else ret = ERROR_IO_PENDING; } - else ret = read_data( request, buffer, to_read, read, async ); release_object( &request->hdr ); SetLastError( ret ); diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c index 98df1571464..3e67c439bb5 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1904,6 +1904,9 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex break; case WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE: + { + DWORD len; + if (!context->read_from_callback) { SetEvent( context->wait ); @@ -1920,13 +1923,18 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex "got %lu, thread %#lx\n", context->recursion_count, GetCurrentThreadId() ); context->max_recursion_query = max( context->max_recursion_query, context->recursion_count ); InterlockedIncrement( &context->recursion_count ); - ret = WinHttpReadData( context->request, &b, 1, NULL ); + b = 0xff; + len = 0xdeadbeef; + ret = WinHttpReadData( context->request, &b, 1, &len ); err = GetLastError(); ok( ret, "failed to read data, GetLastError() %lu\n", err ); ok( err == ERROR_SUCCESS || err == ERROR_IO_PENDING, "got %lu\n", err ); + ok( b != 0xff, "got %#x.\n", b ); + ok( len == 1, "got %lu.\n", len ); if (err == ERROR_SUCCESS) context->have_sync_callback = TRUE; InterlockedDecrement( &context->recursion_count ); break; + } case WINHTTP_CALLBACK_STATUS_READ_COMPLETE: { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4925
v3: - The last test run showed that the added tests are actually flaky. But given that buffer output did not fail and only length randomly got 0xdeadbeef I think it is the race on static variable usage (indicating some callbacks may race on Windows in such a process). I deduplicated the variable between callbacks and made it non-static for the case where static variable is not needed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4925#note_59017
This merge request was approved by Hans Leidekker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4925
participants (3)
-
Hans Leidekker (＠hans) -
Paul Gofman
-
Paul Gofman (＠gofman)