Re: [Bug-wget] [Bug-Wget] Wget closes connection despite Keep-Alive

[Darshit Shah]

On 11/04, Tim Rühsen wrote:
> Am Dienstag, 4. November 2014, 23:25:40 schrieb Darshit Shah:
> > While looking at some debug output from Wget, I noticed that in case of a
> > 416 Range Not Satisfiable response, Wget forces the connection close
> > despite the fact that the server explicitly sent a `Connection: Keep-Alive`
> > header.
> >
> > Looking at the code, I found this at http.c:2775
> >
> >
> >       CLOSE_INVALIDATE (sock);        /* would be CLOSE_FINISH, but there
> >                                    might be more bytes in the body. */
> >
> > I'm not sure what exactly we're trying to protect in this case. Why is
> > CLOSE_INVALIDATE required instead of CLOSE_FINISH? If the server responds
> > with a 416 status, there should be no more data in the stream and we should
> > be able to reuse that connection without any issues.
> >
> > What am I missing here?
>
> The code is with an if construct with || :
> if (statcode == HTTP_STATUS_RANGE_NOT_SATISFIABLE
>      || (!opt.timestamping && hs->restval > 0 && statcode == HTTP_STATUS_OK
>          && contrange == 0 && contlen >= 0 && hs->restval >= contlen))
>    {
>
> So if statcode == HTTP_STATUS_RANGE_NOT_SATISFIABLE i guess we could use
> CLOSE_FINISH. But I can't judge that for the other part of the expression.
> If in doubt, CLOSE_INVALIDATE might be a better choice than CLOSE_FINISH.
> Wget simply opens a new connection for the next request in this case.
I'm pretty sure that in case of `statcode == HTTP_STATUS_RANGE_NOT_SATISFIABLE` 
we can use CLOSE_FINISH(). The other condition is what I am confused about.

* Off-topic:
Looking at the condition, there's one comparison that we can avoid:
hs->restval > 0
contlen >= 0
hs->restval >= contlen

In this case, hs->restval > 0 is redundant and un-needed.


> > I wanted to write a test case to demonstrate this, but detecting a closed
> > socket will take more than a quick test case. I'll sit down and write the
> > required functionality to generate the relevant test case when I can.
>
> Don't spend too much time here. It is not a bug, more of a missing feature.
> And also a very seldom used one. And how I read the comment, servers might
> behave buggy. And again, in that case it seems more reliable when Wget closes
> the connection and opens a new one.
The reason why I came across this was that I needed Wget to maintain a 
persistent connection, and that was exactly what the server expected. However, 
Wget went ahead and closed the connection. This *missing feature* was precisely 
what I was looking for.

Also, this behaviour will become a bug with SPDY and HTTP/2 since the server 
will try to use the open connection to suggest other resources which the client 
may ask for.


I'm waiting for Giuseppe's views on this. Else, I'll split the condition into 
two parts, with the first one called CLOSE_FINISH instead of CLOSE_INVALIDATE


--
Thanking You,
Darshit Shah

pgpEfWILOQPfj.pgp
Description: PGP signature