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
