Re: wininet: Don't assume that end of chunk means end of stream.

[Jacek Caban]

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

wininet: Don't assume that end of chunk means end of stream.

[Hans Leidekker]

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.

wininet: Don't assume that end of chunk means end of stream.

[Jacek Caban]

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

wininet: Don't assume that end of chunk means end of stream.

[Hans Leidekker]

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.

wininet: Don't assume that end of chunk means end of stream.

[Jacek Caban]

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