From 22e3e12b43a8cf0fdb3a474fd5800c4db496423d Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 11:32:12 +0200 Subject: http: Always Close() the connection in Die() If we reached Die() there was an issue with the server connection, so we should always explicitly close it. --- methods/http.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 1d2c41337..76d4f6b5a 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -700,6 +700,8 @@ ResultState HttpServerState::Die(RequestState &Req) { unsigned int LErrno = errno; + Close(); + // Dump the buffer to the file if (Req.State == RequestState::Data) { @@ -727,7 +729,6 @@ ResultState HttpServerState::Die(RequestState &Req) if (In.IsLimit() == false && Req.State != RequestState::Header && Persistent == true) { - Close(); if (LErrno == 0) { _error->Error(_("Error reading from server. Remote end closed connection")); @@ -746,7 +747,6 @@ ResultState HttpServerState::Die(RequestState &Req) return ResultState::TRANSIENT_ERROR; // We may have got multiple responses back in one packet.. - Close(); return ResultState::SUCCESSFUL; } -- cgit v1.2.3 From 1ee3b64be054a8deb6a598d93047637448e1928e Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 11:37:29 +0200 Subject: http: Die(): Merge flushing code from Flush() Die() needs its own Copy() of Flush() because it needs to return success or failure based on some states, but those are not precisely the same as Flush(), as Flush() will always return false at the end, for example, but we want to fall through to our error handling. --- methods/http.cc | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 76d4f6b5a..fef719e74 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -711,6 +711,8 @@ ResultState HttpServerState::Die(RequestState &Req) // can't be set if (Req.File.Name() != "/dev/null") SetNonBlock(Req.File.Fd(),false); + if (not In.WriteSpace()) + return ResultState::SUCCESSFUL; while (In.WriteSpace() == true) { if (In.Write(MethodFd::FromFd(Req.File.Fd())) == false) @@ -723,6 +725,9 @@ ResultState HttpServerState::Die(RequestState &Req) if (In.IsLimit() == true) return ResultState::SUCCESSFUL; } + + if (In.IsLimit() == true || Persistent == false) + return ResultState::SUCCESSFUL; } // See if this is because the server finished the data stream -- cgit v1.2.3 From cb743d117bcc666dab4c5948b1227ed2edbd0578 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 11:45:45 +0200 Subject: http: Only return false for EOF if we actually did not read anything This should avoid the need to Flush the buffer in Die(), because if we read anything, we are returning true, and not entering Die() at that point. Also Write() does not have a concept of EOF, so get rid of code handling that there. Was that copied from Read()? --- methods/http.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index fef719e74..83f1bde8d 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -94,6 +94,7 @@ void CircleBuf::Reset() is non-blocking.. */ bool CircleBuf::Read(std::unique_ptr const &Fd) { + size_t ReadThisCycle = 0; while (1) { // Woops, buffer is full @@ -131,7 +132,7 @@ bool CircleBuf::Read(std::unique_ptr const &Fd) CircleBuf::BwTickReadData += Res; if (Res == 0) - return false; + return ReadThisCycle != 0; if (Res < 0) { if (errno == EAGAIN) @@ -140,6 +141,7 @@ bool CircleBuf::Read(std::unique_ptr const &Fd) } InP += Res; + ReadThisCycle += Res; } } /*}}}*/ @@ -204,8 +206,6 @@ bool CircleBuf::Write(std::unique_ptr const &Fd) ssize_t Res; Res = Fd->Write(Buf + (OutP % Size), LeftWrite()); - if (Res == 0) - return false; if (Res < 0) { if (errno == EAGAIN) @@ -215,7 +215,7 @@ bool CircleBuf::Write(std::unique_ptr const &Fd) } TotalWriten += Res; - + if (Hash != NULL) Hash->Add(Buf + (OutP%Size),Res); -- cgit v1.2.3 From 24d308455a5f8751f57219f211a5672af340099e Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 11:48:28 +0200 Subject: http: Die(): Do not flush the buffer, error out instead By changing the buffer implementation to return true if it read or wrote something, even on EOF, we should not have a need to flush the buffer in Die() anymore - we should only be calling Die() if the buffer is empty now. --- methods/http.cc | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 83f1bde8d..40a37a8c6 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -705,29 +705,14 @@ ResultState HttpServerState::Die(RequestState &Req) // Dump the buffer to the file if (Req.State == RequestState::Data) { - if (Req.File.IsOpen() == false) - return ResultState::SUCCESSFUL; // on GNU/kFreeBSD, apt dies on /dev/null because non-blocking // can't be set if (Req.File.Name() != "/dev/null") SetNonBlock(Req.File.Fd(),false); - if (not In.WriteSpace()) - return ResultState::SUCCESSFUL; - while (In.WriteSpace() == true) - { - if (In.Write(MethodFd::FromFd(Req.File.Fd())) == false) - { - _error->Errno("write", _("Error writing to the file")); - return ResultState::TRANSIENT_ERROR; - } - - // Done - if (In.IsLimit() == true) - return ResultState::SUCCESSFUL; + if (In.WriteSpace()) { + _error->Error(_("Data left in buffer")); + return ResultState::TRANSIENT_ERROR; } - - if (In.IsLimit() == true || Persistent == false) - return ResultState::SUCCESSFUL; } // See if this is because the server finished the data stream -- cgit v1.2.3 From 9742032dcdc0e72c117ae0c589fbb59452d6d33c Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 12:23:02 +0200 Subject: http: Finish copying data from server to file before sending stuff to server This avoids a case where we read data, then write to the server and only then realize the connection was closed. It is somewhat slower, though. --- methods/http.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 40a37a8c6..161ecf067 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -846,13 +846,6 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) return Die(Req); } - if (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &wfds)) - { - errno = 0; - if (Out.Write(ServerFd) == false) - return Die(Req); - } - // Send data to the file if (FileFD->Fd() != -1 && FD_ISSET(FileFD->Fd(), &wfds)) { @@ -863,6 +856,13 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) } } + if (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &wfds)) + { + errno = 0; + if (Out.Write(ServerFd) == false) + return Die(Req); + } + if (Req.MaximumSize > 0 && Req.File.IsOpen() && Req.File.Failed() == false && Req.File.Tell() > Req.MaximumSize) { Owner->SetFailReason("MaximumSizeExceeded"); -- cgit v1.2.3 From c2cb8abbf5d8a49b25071ffffca93a083fe725fc Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 12:31:55 +0200 Subject: http: On select timeout, error out directly, do not call Die() The error handling in Die() that's supposed to add useful error messages is not super useful here. --- methods/http.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 161ecf067..c8aeea1ad 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -835,7 +835,7 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) if (Res == 0) { _error->Error(_("Connection timed out")); - return Die(Req); + return ResultState::TRANSIENT_ERROR; } // Handle server IO -- cgit v1.2.3 From 08f05aa8beb58fa32485e2087eb21a9f3cb267bb Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 29 Jun 2020 14:03:21 +0200 Subject: http: Redesign reading of pending data Instead of reading the data early, disable the timeout for the select() call and read the data later. Also, change Read() to call only once to drain the buffer in such instances. We could optimize this to call read() multiple times if there is also pending stuff on the socket, but that it slightly more complex and should not provide any benefits. --- methods/http.cc | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index c8aeea1ad..9cfc91330 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -783,13 +783,11 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) ToFile == false)) return ResultState::TRANSIENT_ERROR; - // Handle server IO - if (ServerFd->HasPending() && In.ReadSpace() == true) - { - errno = 0; - if (In.Read(ServerFd) == false) - return Die(Req); - } + // Record if we have data pending to read in the server, so that we can + // skip the wait in select(). This can happen if data has already been + // read into a methodfd's buffer - the TCP queue might be empty at that + // point. + bool ServerPending = ServerFd->HasPending(); fd_set rfds,wfds; FD_ZERO(&rfds); @@ -821,7 +819,7 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) // Select struct timeval tv; - tv.tv_sec = TimeOut; + tv.tv_sec = ServerPending ? 0 : TimeOut; tv.tv_usec = 0; int Res = 0; if ((Res = select(MaxFd+1,&rfds,&wfds,0,&tv)) < 0) @@ -832,14 +830,14 @@ ResultState HttpServerState::Go(bool ToFile, RequestState &Req) return ResultState::TRANSIENT_ERROR; } - if (Res == 0) + if (Res == 0 && not ServerPending) { _error->Error(_("Connection timed out")); return ResultState::TRANSIENT_ERROR; } // Handle server IO - if (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &rfds)) + if (ServerPending || (ServerFd->Fd() != -1 && FD_ISSET(ServerFd->Fd(), &rfds))) { errno = 0; if (In.Read(ServerFd) == false) -- cgit v1.2.3