Re: [Bug-wget] [Bug-Wget] Patch Test-proxied-https-auth.px

[Giuseppe Scrivano]

Darshit Shah <address@hidden> writes:

> On 10/31, Giuseppe Scrivano wrote:
>>Darshit Shah <address@hidden> writes:
>>
>>> Ever since the Perl tests have been ported to the parallel test
>>> harness, Test-proxied-https-auth.px has always failed for me. In a
>>> prior thread, we'd identified the problem as a race between the HTTPS
>>> server being set-up and Wget sending the request.
>>>
>>> The test was already using `sleep 1` to eliminate this race. I've
>>> bumped this up to 2 seconds. Now on my machine, the test no longer
>>> fails.
>>>
>>> If everyone is okay with this change, lets push it. If someone wants
>>> to change the number, please speak up.
>>
>>the problem seems to be that sometimes wget registers a connection as
>>persistent even though it should not that.
>>
>>Before I prepare a patch, I would like to check with you all as you
>>already spent some time on this issue.
>>
>>This code dates back to 2003, but I am not sure we should check for the
>>"Connection" response header only if there is a Content-Length specified
>>(that is contlen != -1).
>>
>>With this patch I could run the test in a loop without failures.
>>
>>(while true; do make check -C tests TESTS=Test-proxied-https-auth || exit; 
>>done)
>>
>>diff --git a/src/http.c b/src/http.c
>>index 844f800..cded2de 100644
>>--- a/src/http.c
>>+++ b/src/http.c
>>@@ -2287,7 +2287,7 @@ read_header:
>>     }
>>
>>   /* Check for keep-alive related responses. */
>>-  if (!inhibit_keep_alive && contlen != -1)
>>+  if (!inhibit_keep_alive)
>>     {
>>       if (resp_header_copy (resp, "Connection", hdrval, sizeof (hdrval)))
>>         {
>>
>>
>
> This patch does fix the issue for me too. I agree that the problem was
> in how Wget creates persistent connections. Also, I don't think that
> we should check for the Connection header *only* if the Content-Length
> exists, there are lots of HTTP/1.1 servers that use a Keep-Alive
> connection without sending the Content-Length.
>
> I ack this patch

thanks.  I will add a changelog entry and push it.

Regards,
Giuseppe