On Tue, Jan 12, 2016 at 01:56:10PM +0000, Robie Basak wrote: > On Tue, Jan 12, 2016 at 02:36:28PM +0100, Julian Andres Klode wrote: > > Well, yes, nobody really uses HTTP/1.0 servers, so it's not really tested > > much. > > I just checked and a newer squid (3.1.19) that I have handy also > responds with HTTP/1.0. I don't have a squid 3.4 deployment to check, > but 3.1 shipped in wheezy (Debian LTS project EOL is 2018) and in Ubuntu > 12.04, which is not EOL until 2019. I think apt needs to be able to work > by default through squid proxies that are still commonly deployed. > > > If we have hashes, we will try to do pipelining and then fall back if > > the server messes up the response. > > > > Maybe it helps to also disable pipelining if the server responds with > > HTTP/1.0, like this: > > I think this would help. You will reduce the number of pipelined > requests, and thus the number of times the race is lost. But you won't > eliminate the race completely. Really you need to only enable pipelining > after you see an HTTP/1.1 or higher response, rather than the other way > round. That is trickier to code correctly but is required by the > standards, AIUI. >
Fixed in my branch in this commit: https://github.com/Debian/apt/commit/2caf777bf7badbc75e6503c4dcee71e51b0a43d8 just need to fix a failing test case that checks if pipelining is messed up, as that now does no pipelining for the first package anymore. And change the variable name to something nicer. diff --git a/methods/server.cc b/methods/server.cc index d5188d4..9f21eff 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -150,9 +150,15 @@ bool ServerState::HeaderLine(string Line) else { if (Major == 1 && Minor == 0) + { Persistent = false; + } else + { Persistent = true; + if (ServerIsDoingPipeliningWrong == false) + Pipeline = true; + } } return true; @@ -240,7 +246,7 @@ bool ServerState::HeaderLine(string Line) } /*}}}*/ // ServerState::ServerState - Constructor /*{{{*/ -ServerState::ServerState(URI Srv, ServerMethod *Owner) : ServerName(Srv), TimeOut(120), Owner(Owner) +ServerState::ServerState(URI Srv, ServerMethod *Owner) : ServerName(Srv), TimeOut(120), Owner(Owner), ServerIsDoingPipeliningWrong(false) { Reset(); } @@ -532,6 +538,7 @@ int ServerMethod::Loop() _error->Discard(); Server->Close(); Server->Pipeline = false; + Server->ServerIsDoingPipeliningWrong = true; if (FailCounter >= 2) { @@ -604,6 +611,7 @@ int ServerMethod::Loop() strprintf(out, _("Automatically disabled %s due to incorrect response from server/proxy. (man 5 apt.conf)"), "Acquire::http::PipelineDepth"); std::cerr << "W: " << out << std::endl; Server->Pipeline = false; + Server->ServerIsDoingPipeliningWrong = true; // we keep the PipelineDepth value so that the rest of the queue can be fixed up as well } Rename(Res.Filename, I->DestFile); diff --git a/methods/server.h b/methods/server.h index 365bc08..8f81090 100644 --- a/methods/server.h +++ b/methods/server.h @@ -51,6 +51,7 @@ struct ServerState enum {Chunked,Stream,Closes} Encoding; enum {Header, Data} State; bool Persistent; + bool ServerIsDoingPipeliningWrong; std::string Location; // This is a Persistent attribute of the server itself. @@ -86,7 +87,7 @@ struct ServerState bool Comp(URI Other) const {return Other.Host == ServerName.Host && Other.Port == ServerName.Port;}; virtual void Reset() {Major = 0; Minor = 0; Result = 0; Code[0] = '\0'; TotalFileSize = 0; JunkSize = 0; StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; - State = Header; Persistent = false; Pipeline = true; MaximumSize = 0;}; + State = Header; Persistent = false; Pipeline = false; MaximumSize = 0;}; virtual bool WriteResponse(std::string const &Data) = 0; /** \brief Transfer the data from the socket */ -- Julian Andres Klode - Debian Developer, Ubuntu Member See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/. When replying, only quote what is necessary, and write each reply directly below the part(s) it pertains to (`inline'). Thank you.
