[PATCH v2 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. -- v2: 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 | 7 +++- dlls/winhttp/winhttp_private.h | 13 +++++++ 3 files changed, 73 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..40db323fb5e 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1863,6 +1863,7 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex DWORD status, void *buffer, DWORD buflen ) { struct test_recursion_context *context = (struct test_recursion_context *)context_ptr; + static DWORD len; DWORD err; BOOL ret; BYTE b; @@ -1939,10 +1940,14 @@ 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; 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 | 6 ++++- 2 files changed, 44 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 40db323fb5e..ed70ed906f0 100644 --- a/dlls/winhttp/tests/notification.c +++ b/dlls/winhttp/tests/notification.c @@ -1921,10 +1921,14 @@ 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4925
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142366 Your paranoid android. === w7u_2qxl (32 bit report) === winhttp: notification.c:1931: Test failed: got 3735928559. === w7u_el (32 bit report) === winhttp: notification.c:1931: Test failed: got 3735928559. === w10pro64 (32 bit report) === winhttp: notification.c:1931: Test failed: got 3735928559. === w10pro64 (64 bit report) === winhttp: notification.c:1931: Test failed: got 99. === w10pro64_ar (64 bit report) === winhttp: notification.c:1931: Test failed: got 3735928559.
v2: - renamed data_available -> wont_block. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4925#note_59014
participants (3)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)