diff --git a/Common/Net/HTTPNaettRequest.cpp b/Common/Net/HTTPNaettRequest.cpp index f84accc98e..5d61427bd0 100644 --- a/Common/Net/HTTPNaettRequest.cpp +++ b/Common/Net/HTTPNaettRequest.cpp @@ -1,6 +1,9 @@ #ifndef HTTPS_NOT_AVAILABLE +#include #include +#include +#include #include "Common/Net/HTTPRequest.h" #include "Common/Net/HTTPNaettRequest.h" @@ -20,6 +23,48 @@ HTTPSRequest::~HTTPSRequest() { HTTPSRequest::Join(); } +// The response body, and the flag that stops it arriving. naett hands us chunks on its own +// transfer thread, and there's no way to make it stop and be sure it has: naettClose only really +// cancels on Android, and waiting for the others would mean blocking shutdown. So the buffer it +// writes into is refcounted separately from the request - if we go away first, the sink stays +// alive and a late chunk lands somewhere harmless instead of in a destroyed object. +struct NaettBodySink { + Buffer buffer; + int length = 0; + // Written by us, read by the transfer thread. + std::atomic cancelled{false}; +}; + +// Sinks belonging to requests that hadn't finished when they were torn down, which only happens +// at shutdown - RequestManager cancels from its destructor. The naett objects can't be freed +// while a callback might still be in flight, and neither can these, so both are deliberately +// leaked. Allocated with new and never deleted, so it can't be destroyed out from under a late +// callback during static destruction either. +static std::vector> *g_abandonedSinks = new std::vector>(); + +int HTTPSRequest::WriteBodyThunk(const void *source, int bytes, void *userData) { + NaettBodySink *sink = (NaettBodySink *)userData; + if (sink->cancelled) { + // Taking less than we were given fails the request, which is how naett lets us stop a + // transfer. Without this, cancelling only relabelled the result once it finished anyway. + return 0; + } + if (bytes <= 0) { + return 0; + } + char *dest = sink->buffer.Append((size_t)bytes); + memcpy(dest, source, bytes); + sink->length += bytes; + return bytes; +} + +void HTTPSRequest::Cancel() { + Request::Cancel(); + if (sink_) { + sink_->cancelled = true; + } +} + void HTTPSRequest::Start() { _dbg_assert_(!req_); _dbg_assert_(!res_); @@ -41,6 +86,10 @@ void HTTPSRequest::Start() { } // 30 s timeout - not sure what's reasonable? options.push_back(naettTimeout(30 * 1000)); // milliseconds + // Our own writer, so that Cancel() can actually stop a transfer rather than just relabelling + // it once it finishes. + sink_ = std::make_shared(); + options.push_back(naettBodyWriter(&HTTPSRequest::WriteBodyThunk, sink_.get())); const naettOption **opts = (const naettOption **)options.data(); req_ = naettRequestWithOptions(url_.c_str(), (int)options.size(), opts); @@ -52,15 +101,28 @@ void HTTPSRequest::Start() { void HTTPSRequest::Join() { if (!res_ || !req_) return; // No pending operation. - // Tear down. - if (completed_) { - _dbg_assert_(req_); + // Tear down. A request that finished while nobody was polling Done() can still be closed + // properly - it's only one that's genuinely still running that can't be. + if (completed_ || naettComplete(res_)) { naettClose(res_); naettFree(req_); res_ = nullptr; req_ = nullptr; + sink_.reset(); } else { - ERROR_LOG(Log::HTTP, "HTTPSRequest::Join called before completion"); + // Only reachable at shutdown, since RequestManager cancels from its destructor and + // otherwise waits for Done(). Closing a response naett is still working on isn't safe on + // three of the four backends, and there's nothing of ours to wait on, so let the request + // go and keep its sink alive - a chunk arriving after this point then writes somewhere + // that still exists. The process is on its way out; this is the last word on it. + WARN_LOG(Log::HTTP, "Abandoning an unfinished request to '%s' - shutting down", url_.c_str()); + if (sink_) { + sink_->cancelled = true; + g_abandonedSinks->push_back(sink_); + sink_.reset(); + } + res_ = nullptr; + req_ = nullptr; } } @@ -79,10 +141,11 @@ bool HTTPSRequest::Done() { // -1000 is a code specified by us to represent cancellation, that is unlikely to ever collide with naett error codes. resultCode_ = IsCancelled() ? -1000 : naettGetStatus(res_); - int bodyLength; - const void *body = naettGetBody(res_, &bodyLength); - char *dest = buffer_.Append(bodyLength); - memcpy(dest, body, bodyLength); + // The body arrived in the sink as it was read; take it over now that nothing else will touch it. + const int bodyLength = sink_ ? sink_->length : 0; + if (sink_ && bodyLength > 0) { + buffer_.Append(sink_->buffer); + } if (resultCode_ < 0) { // It's a naett error. Translate and handle. switch (resultCode_) { diff --git a/Common/Net/HTTPNaettRequest.h b/Common/Net/HTTPNaettRequest.h index 9d1a4d95b0..7be0527e90 100644 --- a/Common/Net/HTTPNaettRequest.h +++ b/Common/Net/HTTPNaettRequest.h @@ -24,12 +24,23 @@ public: bool Done() override; bool Failed() const override { return failed_; } + // Cancelling has to reach naett, or it only changes the code we report once the transfer ends + // on its own. See the .cpp. + void Cancel() override; + private: + static int WriteBodyThunk(const void *source, int bytes, void *userData); + std::string postData_; std::string postMime_; bool completed_ = false; bool failed_ = false; + // Where the response body lands. Deliberately not a member of this object: naett writes into + // it from its own transfer thread, and that can outlive us if we're torn down before the + // request finishes. See NaettBodySink in the .cpp. + std::shared_ptr sink_; + // Naett state naettReq *req_ = nullptr; naettRes *res_ = nullptr; diff --git a/Common/Net/HTTPRequest.h b/Common/Net/HTTPRequest.h index 29991b49ac..0cd8db3288 100644 --- a/Common/Net/HTTPRequest.h +++ b/Common/Net/HTTPRequest.h @@ -75,7 +75,9 @@ public: flags_ |= flag; } - void Cancel() { cancelled_ = true; } + // Virtual so a backend can act on it. The HTTPS one has to tell naett, which is what actually + // stops a transfer in progress - see HTTPSRequest::Cancel. + virtual void Cancel() { cancelled_ = true; } bool IsCancelled() const { return cancelled_; } // If not downloading to a file, access this to get the result.