Bug#810796: HTTP pipelining is broken and causes download failures
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.
Reply to: