Rewrite HttpServerState::Die()

The old code was fairly confusing, and contradictory. Notably, the
second `if` also only applied to the Data state, whereas we already
terminated the Data state earlier. This was bad.

The else fallback applied in three cases:

(1) We reached our limit
(2) We are Persistent
(3) We are headers

Now, it always failed as a transient error if it had
nothing left in the buffer. BUT: Nothing left in the buffer
is the correct thing to happen if we were fetching content.

Checking all combinations for the flags, we can compare the results
of Die() between 2.1.7 - the last "known-acceptable-ish" version
and this version:
                                2.1.7           this
Data !Persist !Space !Limit     OK (A)           OK
Data !Persist !Space Limit      OK (A)           OK
Data !Persist Space !Limit      OK (C)           OK
Data !Persist Space Limit       OK               OK

Data Persist !Space !Limit      ERR              ERR          *
Data Persist !Space Limit       OK (B)           OK
Data Persist Space !Limit       ERR              ERR
Data Persist Space Limit        OK               OK

=> Data connections are OK if they have not reached their limit,
   or are persistent (in which case they'll probably be chunked)

Header !Persist !Space !Limit   ERR              ERR
Header !Persist !Space Limit    ERR              ERR
Header !Persist Space !Limit    OK               OK
Header !Persist Space Limit     OK               OK
Header Persist !Space !Limit    ERR              ERR
Header Persist !Space Limit     ERR              ERR
Header Persist Space !Limit     OK               OK
Header Persist Space Limit      OK               OK

=> Common scheme here is that header connections are fine if they have
   read something into the input buffer (Space). The rest does not matter.

(A) Non-persistent connections with !space always enter the else clause, hence  success
(B) no Space means we enter the if/else, we go with else because IsLimit(), and we succeed because we don't have space
(C) Having space we do enter the while (WriteSpace()) loop, but we never reach IsLimit(),
    hence we fall through. Given that our connection is not persistent, we fall through to the
    else case, and there we win because we have data left to write.

1 files changed, 18 insertions, 28 deletions

diff --git a/methods/http.cc b/methods/http.cc
index fb09bddd7..0ef695166 100644
--- a/methods/http.cc
+++ b/methods/http.cc
@@ -702,40 +702,30 @@ ResultState HttpServerState::Die(RequestState &Req)
 
    Close();
 
-   // Dump the buffer to the file
-   if (Req.State == RequestState::Data)
+   switch (Req.State)
    {
-      if (not In.WriteSpace() || In.IsLimit() == true || Persistent == false)
-        return ResultState::SUCCESSFUL;
-
-      _error->Error(_("Data left in buffer"));
-      return ResultState::TRANSIENT_ERROR;
+   case RequestState::Data:
+      // We have read all data we could, or the connection is not persistent
+      if (In.IsLimit() == true || Persistent == false)
+	 return ResultState::SUCCESSFUL;
+      break;
+   case RequestState::Header:
+      In.Limit(-1);
+      // We have read some headers, but we might also have read the content
+      // and an EOF and hence reached this point. This is fine.
+      if (In.WriteSpace())
+	 return ResultState::SUCCESSFUL;
+      break;
    }
 
-   // See if this is because the server finished the data stream
-   if (In.IsLimit() == false && Req.State != RequestState::Header &&
-       Persistent == true)
+   // We have reached an actual error, tell the user about it.
+   if (LErrno == 0)
    {
-      if (LErrno == 0)
-      {
-	 _error->Error(_("Error reading from server. Remote end closed connection"));
-	 return ResultState::TRANSIENT_ERROR;
-      }
-      errno = LErrno;
-      _error->Errno("read", _("Error reading from server"));
+      _error->Error(_("Error reading from server. Remote end closed connection"));
       return ResultState::TRANSIENT_ERROR;
    }
-   else
-   {
-      In.Limit(-1);
-
-      // Nothing left in the buffer
-      if (In.WriteSpace() == false)
-	 return ResultState::TRANSIENT_ERROR;
-
-      // We may have got multiple responses back in one packet..
-      return ResultState::SUCCESSFUL;
-   }
+   errno = LErrno;
+   _error->Errno("read", _("Error reading from server"));
 
    return ResultState::TRANSIENT_ERROR;
 }