Re: wininet: Don't assume that end of chunk means end of stream. (try 2)
Hi Hans,
Sorry for the delay, I wanted to test it a bit more before answering.
On 09/23/13 10:50, Hans Leidekker wrote:
> On Thu, 2013-09-19 at 17:38 +0200, Jacek Caban wrote:
>> I was hoping for a test like the attached one, which shows that there is
>> still more work to do. But it's a step in the right direction, so I'm
>> fine with your patch.
> The issue here is that HTTP_ReceiveRequestData calls refill_read_buffer, which
> calls generic read_http_stream with a read size of 8192 (the read buffer
> is still empty). In non-chunked mode this should read the minimum of that size
> and the content length. In chunked mode we don't know the content length,
> and we should read the minimum of chunk size and read buffer size, as shown by
> your test.
> Maybe we should add a refill_read_buffer method to the backends and call that
> instead of read_http_stream?
We should be able to do that with read semantics that is intended for
READMODE_ASYNC: read anything, not more than buf size and, once any data
is read, don't block. The attached extended tests show the problem with
chunked reads in this case. We'd need to maintain full state of the
stream to be able to do non-blocking chunk headers/tails reads.
Jacek
diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c
index 12eab57..889707c 100644
--- a/dlls/wininet/tests/http.c
+++ b/dlls/wininet/tests/http.c
@@ -103,7 +103,7 @@ static int expect[MAX_INTERNET_STATUS],
optional[MAX_INTERNET_STATUS],
wine_allow[MAX_INTERNET_STATUS], notified[MAX_INTERNET_STATUS];
static const char *status_string[MAX_INTERNET_STATUS];
-static HANDLE hCompleteEvent, conn_close_event;
+static HANDLE hCompleteEvent, conn_close_event, server_event;
static DWORD req_error;
#define TESTF_REDIRECT 0x01
@@ -2114,6 +2114,36 @@ static DWORD CALLBACK server_thread(LPVOID param)
}
if (strstr(buffer, "GET /test_premature_disconnect"))
trace("closing connection\n");
+if (strstr(buffer, "GET /test_chunked")) {
+static const char chunked_header[] = "HTTP/1.1 200
OK\r\nTransfer-Encoding: chunked\r\n\r\n";
+static const char chunk1[] =
"20\r\n\r\n";
+static const char chunk2[] = "18\r\n\r\n";
+static const char chunk3[] =
"28\r\n\r\n";
+static const char last_chunk[] = "0\r\n\r\n";
+
+send(c, chunked_header, sizeof(chunked_header)-1, 0);
+send(c, chunk1, sizeof(chunk1)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk2, sizeof(chunk2)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk3, sizeof(chunk3)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk1, sizeof(chunk1)-1, 0);
+send(c, chunk2, sizeof(chunk2)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk1, sizeof(chunk1)-1, 0);
+send(c, chunk2, 3, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk2+3, sizeof(chunk2)-4, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, last_chunk, sizeof(last_chunk)-1, 0);
+}
shutdown(c, 2);
closesocket(c);
@@ -2917,6 +2947,181 @@ static void test_no_content(int port)
CloseHandle(hCompleteEvent);
}
+static void test_chunked_stream(int port)
+{
+HINTERNET session, connection, req;
+DWORD res, avail, size;
+BYTE buf[256];
+
+trace("Testing chunked stream...\n");
+
+hCompleteEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
+server_event = CreateEvent(NULL, FALSE, FALSE, NULL);
+
+session = InternetOpenA("", INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL,
INTERNET_FLAG_ASYNC);
+ok(session != NULL,"InternetOpen failed with error %u\n", GetLastError());
+
+pInternetSetStatusCallbackA(session, callback);
+
+SET_EXPECT(INTERNET_STATUS_HANDLE_CREATED);
+connection = InternetConnectA(session, "localhost", port,
+NULL, NULL, INTERNET_SERVICE_HTTP, 0x0, 0xdeadbeef);
+ok(connection != NULL,"InternetConnect failed with error %u\n",
GetLastError());
+CHECK_NOTIFIED(INTERNET_STATUS_HANDLE_CREATED);
+
+SET_EXPECT(INTERNET_STATUS_HANDLE_CREATED);
+req = HttpOpenRequestA(connection, "GET", "/test_chunked", NULL, NULL,
NULL,
+INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_RESYNCHRONIZE,
0xdeadbead);
+ok(req != NULL, "HttpOpenRequest failed: %u\n", GetLastError());
+CHECK_NOTIFIED(INTERNET_STATUS_HANDLE_CREATED);
+
+trace("sending request...\n");
+
+SET_OPTIONAL(INTERNET_STATUS_COOKIE_SENT);
+SET_EXPECT(INTERNET_STATUS_CONNECTING_TO_SERVER);
+SET_EXPECT(INTERNET_STATUS_CONNECTED_TO_SERVER);
+SET_EXPECT(INTERNET_STATUS_SENDING_REQUEST);
+SET_EXPECT(INTERNET_STATUS_REQUEST_SENT);
+
Re: wininet: Don't assume that end of chunk means end of stream. (try 2)
On Thu, 2013-09-19 at 17:38 +0200, Jacek Caban wrote: > I was hoping for a test like the attached one, which shows that there is > still more work to do. But it's a step in the right direction, so I'm > fine with your patch. The issue here is that HTTP_ReceiveRequestData calls refill_read_buffer, which calls generic read_http_stream with a read size of 8192 (the read buffer is still empty). In non-chunked mode this should read the minimum of that size and the content length. In chunked mode we don't know the content length, and we should read the minimum of chunk size and read buffer size, as shown by your test. Maybe we should add a refill_read_buffer method to the backends and call that instead of read_http_stream?
Re: wininet: Don't assume that end of chunk means end of stream. (try 2)
Hi, While running your changed tests on Windows, 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://newtestbot.winehq.org/JobDetails.pl?Key=2255 Your paranoid android. === build (build) === Patch failed to apply
Re: wininet: Don't assume that end of chunk means end of stream. (try 2)
Hi Hans,
On 09/18/13 13:40, Hans Leidekker wrote:
> diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c
> index bf57223..2526681 100644
> --- a/dlls/wininet/tests/http.c
> +++ b/dlls/wininet/tests/http.c
> @@ -4777,8 +4777,37 @@ static const struct notification
> async_send_request_ex_resolve_failure_test[] =
> { internet_close_handle, INTERNET_STATUS_HANDLE_CLOSING, 0, }
> };
>
> +static const struct notification async_send_request_ex_chunked_test[] =
> +{
> +{ internet_connect, INTERNET_STATUS_HANDLE_CREATED },
> +{ http_open_request, INTERNET_STATUS_HANDLE_CREATED },
> +{ http_send_request_ex, INTERNET_STATUS_DETECTING_PROXY, 1, 0, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_COOKIE_SENT, 1, 0, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_RESOLVING_NAME, 1, 0, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_NAME_RESOLVED, 1, 0, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_CONNECTING_TO_SERVER, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_CONNECTED_TO_SERVER, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_SENDING_REQUEST, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_REQUEST_SENT, 1 },
> +{ http_send_request_ex, INTERNET_STATUS_REQUEST_COMPLETE, 1 },
> +{ http_end_request, INTERNET_STATUS_RECEIVING_RESPONSE, 1 },
> +{ http_end_request, INTERNET_STATUS_RESPONSE_RECEIVED, 1 },
> +{ http_end_request, INTERNET_STATUS_REQUEST_COMPLETE, 1 },
> +{ internet_close_handle, INTERNET_STATUS_CLOSING_CONNECTION },
> +{ internet_close_handle, INTERNET_STATUS_CONNECTION_CLOSED },
> +{ internet_close_handle, INTERNET_STATUS_HANDLE_CLOSING },
> +{ internet_close_handle, INTERNET_STATUS_HANDLE_CLOSING }
> +};
> +
> static const struct notification_data notification_data[] = {
> {
> +async_send_request_ex_chunked_test,
> +
> sizeof(async_send_request_ex_chunked_test)/sizeof(async_send_request_ex_chunked_test[0]),
> +"GET",
> +"test.winehq.org",
> +"tests/data.php"
> +},
> +{
> async_send_request_ex_test,
>
> sizeof(async_send_request_ex_test)/sizeof(async_send_request_ex_test[0]),
> "POST",
> @@ -5100,6 +5129,7 @@ START_TEST(http)
> test_async_HttpSendRequestEx(¬ification_data[0]);
> test_async_HttpSendRequestEx(¬ification_data[1]);
> test_async_HttpSendRequestEx(¬ification_data[2]);
> +test_async_HttpSendRequestEx(¬ification_data[3]);
> InternetOpenRequest_test();
> test_http_cache();
> InternetOpenUrlA_test();
I was hoping for a test like the attached one, which shows that there is
still more work to do. But it's a step in the right direction, so I'm
fine with your patch.
Thanks,
Jacek
commit 39b90723d968f65d1ad11a0c881fb9e7433c6fb3
Author: Jacek Caban
Date: Thu Sep 19 17:18:29 2013 +0200
wininet: Added more chunked stream tests.
diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c
index bf57223..fb67604 100644
--- a/dlls/wininet/tests/http.c
+++ b/dlls/wininet/tests/http.c
@@ -103,7 +103,7 @@ static int expect[MAX_INTERNET_STATUS],
optional[MAX_INTERNET_STATUS],
wine_allow[MAX_INTERNET_STATUS], notified[MAX_INTERNET_STATUS];
static const char *status_string[MAX_INTERNET_STATUS];
-static HANDLE hCompleteEvent, conn_close_event;
+static HANDLE hCompleteEvent, conn_close_event, server_event;
static DWORD req_error;
#define TESTF_REDIRECT 0x01
@@ -2113,6 +2113,25 @@ static DWORD CALLBACK server_thread(LPVOID param)
}
if (strstr(buffer, "GET /test_premature_disconnect"))
trace("closing connection\n");
+if (strstr(buffer, "GET /test_chunked")) {
+static const char chunked_header[] = "HTTP/1.1 200
OK\r\nTransfer-Encoding: chunked\r\n\r\n";
+static const char chunk1[] =
"20\r\n\r\n";
+static const char chunk2[] = "18\r\n\r\n";
+static const char chunk3[] =
"28\r\n\r\n";
+static const char last_chunk[] = "0\r\n\r\n";
+
+send(c, chunked_header, sizeof(chunked_header)-1, 0);
+send(c, chunk1, sizeof(chunk1)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk2, sizeof(chunk2)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, chunk3, sizeof(chunk3)-1, 0);
+WaitForSingleObject(server_event, INFINITE);
+
+send(c, last_chunk, sizeof(last_chunk)-1, 0);
+}
shutdown(c, 2);
closesocket(c);
@@ -2916,6 +2935,130 @@ static void test_no_content(int port)
CloseHandle(hCompleteEvent);
}
+static void test_chunked_stream(int port)
+{
+HINTERNET session, connection, req;
+DWORD res, avail, size;
+BYTE buf[256];
+
+trace("Testing chunked stream...\n");
+
+hCompleteEvent = Create
Re: wininet: Don't assume that end of chunk means end of stream.
On 09/11/13 17:32, Hans Leidekker wrote:
> On Wed, 2013-09-11 at 16:38 +0200, Jacek Caban wrote:
>> Hi Hans,
>>
>> On 09/11/13 13:50, Hans Leidekker wrote:
>>> static DWORD chunked_get_avail_data(data_stream_t *stream, http_request_t
>>> *req)
>>> {
>>> -/* Allow reading only from read buffer */
>>> +chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
>>> +DWORD res;
>>> +
>>> +if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
>>> +res = start_next_chunk(chunked_stream, req);
>>> +if(res != ERROR_SUCCESS)
>>> +return 0;
>>> +}
>> start_next_chunk may block and we don't want chunked_get_avail_data to
>> block.
> We can't avoid that. If this is the first chunk or if the current chunk has
> been consumed we need to read a couple of bytes to find out how much we can
> expect.
The first chunk will be received by HTTP_ReceiveRequestData in both
HTTP_HttpSendRequest and HTTP_HttpEndRequest. Subsequent chunks are also
avoidable, just like they didn't block without your patch. We really
can't block in InternetQueryDataAvailable for async request.
> Network traces tell me that native also performs a read on the first call to
> InternetQueryDataAvailable.
Are you sure it's not an asynchronous read from a separated thread that
is ordered by InternetQueryDataAvailable if no data is available?
Jacek
Re: wininet: Don't assume that end of chunk means end of stream.
On Wed, 2013-09-11 at 17:53 +0200, Jacek Caban wrote: > > Network traces tell me that native also performs a read on the first call to > > InternetQueryDataAvailable. > > Are you sure it's not an asynchronous read from a separated thread that > is ordered by InternetQueryDataAvailable if no data is available? This was a synchronous request.
Re: wininet: Don't assume that end of chunk means end of stream.
On 9/11/13 7:09 PM, Hans Leidekker wrote: On Wed, 2013-09-11 at 17:53 +0200, Jacek Caban wrote: Network traces tell me that native also performs a read on the first call to InternetQueryDataAvailable. Are you sure it's not an asynchronous read from a separated thread that is ordered by InternetQueryDataAvailable if no data is available? This was a synchronous request. For synchronous request, if get_avail_data returns 0, we call blocking refill_read_buffer which may start the new chunk. There is no reason for get_avail_data to block. Also note that that's not the interesting thing to test in regards to chunks handling. The tricky part is async request handling. Jacek
Re: wininet: Don't assume that end of chunk means end of stream.
On Wed, 2013-09-11 at 16:38 +0200, Jacek Caban wrote:
> Hi Hans,
>
> On 09/11/13 13:50, Hans Leidekker wrote:
> > static DWORD chunked_get_avail_data(data_stream_t *stream, http_request_t
> > *req)
> > {
> > -/* Allow reading only from read buffer */
> > +chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
> > +DWORD res;
> > +
> > +if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
> > +res = start_next_chunk(chunked_stream, req);
> > +if(res != ERROR_SUCCESS)
> > +return 0;
> > +}
>
> start_next_chunk may block and we don't want chunked_get_avail_data to
> block.
We can't avoid that. If this is the first chunk or if the current chunk has
been consumed we need to read a couple of bytes to find out how much we can
expect.
Network traces tell me that native also performs a read on the first call to
InternetQueryDataAvailable.
Re: wininet: Don't assume that end of chunk means end of stream.
Hi Hans,
On 09/11/13 13:50, Hans Leidekker wrote:
> static DWORD chunked_get_avail_data(data_stream_t *stream, http_request_t
> *req)
> {
> -/* Allow reading only from read buffer */
> +chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
> +DWORD res;
> +
> +if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
> +res = start_next_chunk(chunked_stream, req);
> +if(res != ERROR_SUCCESS)
> +return 0;
> +}
start_next_chunk may block and we don't want chunked_get_avail_data to
block.
Jacek
