summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJulian Andres Klode <julian.klode@canonical.com>2020-08-11 13:09:14 +0200
committerJulian Andres Klode <julian.klode@canonical.com>2020-08-11 13:42:41 +0200
commit8b35e2a3dd7b863639a8909fa2361ed4fd217bc3 (patch)
tree65dc9e4082948980820923e60565d9ff98fb488a
parent73780d7f664a4ea1da55527d726b4c9c7753f1fb (diff)
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.
-rw-r--r--methods/http.cc46
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;
}