Re: wininet: Don't assume that end of chunk means end of stream. (try 2)

[Jacek Caban]

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(&notification_data[0]);
>      test_async_HttpSendRequestEx(&notification_data[1]);
>      test_async_HttpSendRequestEx(&notification_data[2]);
> - test_async_HttpSendRequestEx(&notification_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

wininet: Don't assume that end of chunk means end of stream. (try 2)

[Hans Leidekker]

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?

wininet: Don't assume that end of chunk means end of stream. (try 2)

[Jacek Caban]

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