From df696650b7a8c58bbd92e0e1619e956f21010a96 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Fri, 31 Aug 2018 16:07:07 +0200
Subject: http: Stop pipeline after close only if it was not filled before

It is perfectly valid behavior for a server to respond with
Connection: close eventually, even when pipelining. Turning
off pipelining due to that is wrong. For example, some Ubuntu
mirrors close the connection after 101 requests. If I have
more packages to install, only the first 101 would benefit
from pipelining.

This commit introduces a new check to only turn of pipelining
for future connections if the pipeline for this connection did
not have 3 successful fetches before, that should work quite well to
detect broken server/proxy combinations like in bug 832113.
---
 methods/basehttp.cc | 24 +++++++++++++++++++-----
 methods/basehttp.h  |  3 ++-
 2 files changed, 21 insertions(+), 6 deletions(-)

(limited to 'methods')

diff --git a/methods/basehttp.cc b/methods/basehttp.cc
index 3d95ba7df..f8dd7c020 100644
--- a/methods/basehttp.cc
+++ b/methods/basehttp.cc
@@ -39,6 +39,10 @@ string BaseHttpMethod::FailFile;
 int BaseHttpMethod::FailFd = -1;
 time_t BaseHttpMethod::FailTime = 0;
 
+// Number of successful requests in a pipeline needed to continue
+// pipelining after a connection reset.
+constexpr int PIPELINE_MIN_SUCCESSFUL_ANSWERS_TO_CONTINUE = 3;
+
 // ServerState::RunHeaders - Get the headers before the data		/*{{{*/
 // ---------------------------------------------------------------------
 /* Returns 0 if things are OK, 1 if an IO error occurred and 2 if a header
@@ -215,8 +219,11 @@ bool RequestState::HeaderLine(string const &Line)			/*{{{*/
 	 /* Some servers send error pages (as they are dynamically generated)
 	    for simplicity via a connection close instead of e.g. chunked,
 	    so assuming an always closing server only if we get a file + close */
-	 if (Result >= 200 && Result < 300)
+	 if (Result >= 200 && Result < 300 && Server->PipelineAnswersReceived < PIPELINE_MIN_SUCCESSFUL_ANSWERS_TO_CONTINUE)
+	 {
 	    Server->PipelineAllowed = false;
+	    Server->PipelineAnswersReceived = 0;
+	 }
       }
       else if (stringcasecmp(Val,"keep-alive") == 0)
 	 Server->Persistent = true;
@@ -267,6 +274,7 @@ void ServerState::Reset()						/*{{{*/
    Pipeline = false;
    PipelineAllowed = true;
    RangesAllowed = true;
+   PipelineAnswersReceived = 0;
 }
 									/*}}}*/
 
@@ -593,8 +601,10 @@ int BaseHttpMethod::Loop()
 	 Server->Close();
 
       // Reset the pipeline
-      if (Server->IsOpen() == false)
+      if (Server->IsOpen() == false) {
 	 QueueBack = Queue;
+	 Server->PipelineAnswersReceived = 0;
+      }
 
       // Connect to the host
       switch (Server->Open())
@@ -752,6 +762,10 @@ int BaseHttpMethod::Loop()
 		     BeforeI = I;
 		  }
 	       }
+	       if (Server->Pipeline == true)
+	       {
+		  Server->PipelineAnswersReceived++;
+	       }
 	       Res.TakeHashes(*resultHashes);
 	       URIDone(Res);
 	    }
@@ -861,9 +875,9 @@ unsigned long long BaseHttpMethod::FindMaximumObjectSizeInQueue() const	/*{{{*/
    return MaxSizeInQueue;
 }
 									/*}}}*/
-BaseHttpMethod::BaseHttpMethod(std::string &&Binary, char const * const Ver,unsigned long const Flags) :/*{{{*/
-   aptAuthConfMethod(std::move(Binary), Ver, Flags), Server(nullptr), PipelineDepth(10),
-   AllowRedirect(false), Debug(false)
+BaseHttpMethod::BaseHttpMethod(std::string &&Binary, char const *const Ver, unsigned long const Flags) /*{{{*/
+    : aptAuthConfMethod(std::move(Binary), Ver, Flags), Server(nullptr),
+      AllowRedirect(false), Debug(false), PipelineDepth(10)
 {
 }
 									/*}}}*/
diff --git a/methods/basehttp.h b/methods/basehttp.h
index 8220c1b3c..5fdff69e0 100644
--- a/methods/basehttp.h
+++ b/methods/basehttp.h
@@ -67,6 +67,7 @@ struct ServerState
    bool Persistent;
    bool PipelineAllowed;
    bool RangesAllowed;
+   unsigned long PipelineAnswersReceived;
 
    bool Pipeline;
    URI ServerName;
@@ -122,7 +123,6 @@ class BaseHttpMethod : public aptAuthConfMethod
    std::unique_ptr<ServerState> Server;
    std::string NextURI;
 
-   unsigned long PipelineDepth;
    bool AllowRedirect;
 
    // Find the biggest item in the fetch queue for the checking of the maximum
@@ -131,6 +131,7 @@ class BaseHttpMethod : public aptAuthConfMethod
 
    public:
    bool Debug;
+   unsigned long PipelineDepth;
 
    /** \brief Result of the header parsing */
    enum DealWithHeadersResult {
-- 
cgit v1.2.3