From 439ae5eceae0b44315d278e963332369f0decfa0 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.

(cherry picked from commit df696650b7a8c58bbd92e0e1619e956f21010a96)
LP: #1794957
(cherry picked from commit 3de7454c796f245371c33076ae01529d6d36d715)
---
 methods/server.cc | 20 +++++++++++++++++---
 methods/server.h  |  5 +++--
 2 files changed, 20 insertions(+), 5 deletions(-)

(limited to 'methods')

diff --git a/methods/server.cc b/methods/server.cc
index cac77e24c..6a7cae2bc 100644
--- a/methods/server.cc
+++ b/methods/server.cc
@@ -40,6 +40,10 @@ string ServerMethod::FailFile;
 int ServerMethod::FailFd = -1;
 time_t ServerMethod::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
@@ -230,8 +234,11 @@ bool ServerState::HeaderLine(string 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 && PipelineAnswersReceived < PIPELINE_MIN_SUCCESSFUL_ANSWERS_TO_CONTINUE)
+	 {
 	    PipelineAllowed = false;
+	    PipelineAnswersReceived = 0;
+	 }
       }
       else if (stringcasecmp(Val,"keep-alive") == 0)
 	 Persistent = true;
@@ -512,7 +519,10 @@ int ServerMethod::Loop()
 
       // Reset the pipeline
       if (Server->IsOpen() == false)
+      {
 	 QueueBack = Queue;
+	 Server->PipelineAnswersReceived = 0;
+      }
 
       // Connnect to the host
       if (Server->Open() == false)
@@ -632,6 +642,10 @@ int ServerMethod::Loop()
 		     BeforeI = I;
 		  }
 	       }
+	       if (Server->Pipeline == true)
+	       {
+		  Server->PipelineAnswersReceived++;
+	       }
 	       Res.TakeHashes(*resultHashes);
 	       URIDone(Res);
 	    }
@@ -759,8 +773,8 @@ unsigned long long ServerMethod::FindMaximumObjectSizeInQueue() const	/*{{{*/
 }
 									/*}}}*/
 ServerMethod::ServerMethod(char const * const Binary, char const * const Ver,unsigned long const Flags) :/*{{{*/
-   aptMethod(Binary, Ver, Flags), Server(nullptr), File(NULL), PipelineDepth(10),
-   AllowRedirect(false), Debug(false)
+   aptMethod(Binary, Ver, Flags), Server(nullptr), File(NULL),
+   AllowRedirect(false), Debug(false), PipelineDepth(10)
 {
 }
 									/*}}}*/
diff --git a/methods/server.h b/methods/server.h
index 28c6851f1..1dd24562e 100644
--- a/methods/server.h
+++ b/methods/server.h
@@ -53,6 +53,7 @@ struct ServerState
    bool Persistent;
    bool PipelineAllowed;
    std::string Location;
+   unsigned long PipelineAnswersReceived;
 
    // This is a Persistent attribute of the server itself.
    bool Pipeline;
@@ -87,7 +88,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 = false; MaximumSize = 0; PipelineAllowed = true;};
+		 State = Header; Persistent = false; Pipeline = false; MaximumSize = 0; PipelineAllowed = true; PipelineAnswersReceived = 0;};
    virtual bool WriteResponse(std::string const &Data) = 0;
 
    /** \brief Transfer the data from the socket */
@@ -115,7 +116,6 @@ class ServerMethod : public aptMethod
    std::string NextURI;
    FileFd *File;
 
-   unsigned long PipelineDepth;
    bool AllowRedirect;
 
    // Find the biggest item in the fetch queue for the checking of the maximum
@@ -124,6 +124,7 @@ class ServerMethod : public aptMethod
 
    public:
    bool Debug;
+   unsigned long PipelineDepth;
 
    /** \brief Result of the header parsing */
    enum DealWithHeadersResult {
-- 
cgit v1.2.3