Merge pull request #22237 from hrydgard/naett-cancel

http: Make Cancel() actually stop an HTTPS transfer

Common/Net/HTTPNaettRequest.cpp

@@ -1,6 +1,9 @@
 #ifndef HTTPS_NOT_AVAILABLE
 
+#include <atomic>
 #include <cstring>
+#include <memory>
+#include <vector>
 
 #include "Common/Net/HTTPRequest.h"
 #include "Common/Net/HTTPNaettRequest.h"
@@ -20,6 +23,47 @@ 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 owned separately from the request, and that ownership can move - if we go away
+// first, the sink is handed off rather than destroyed, and a late chunk lands somewhere that
+// still exists instead of in a destroyed object.
+struct NaettBodySink {
+	Buffer buffer;
+	// Written by us, read by the transfer thread.
+	std::atomic<bool> 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<std::unique_ptr<NaettBodySink>> *g_abandonedSinks = new std::vector<std::unique_ptr<NaettBodySink>>();
+
+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);
+	return bytes;
+}
+
+void HTTPSRequest::Cancel() {
+	Request::Cancel();
+	if (sink_) {
+		sink_->cancelled = true;
+	}
+}
+
 void HTTPSRequest::Start() {
 	_dbg_assert_(!req_);
 	_dbg_assert_(!res_);
@@ -41,10 +85,38 @@ 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_unique<NaettBodySink>();
+	// In case someone managed to cancel us between construction and here.
+	sink_->cancelled = cancelled_;
+	options.push_back(naettBodyWriter(&HTTPSRequest::WriteBodyThunk, sink_.get()));
 
 	const naettOption **opts = (const naettOption **)options.data();
 	req_ = naettRequestWithOptions(url_.c_str(), (int)options.size(), opts);
+	if (!req_) {
+		// naett couldn't set the request up - a URL it can't parse, most likely. Fail it here
+		// rather than handing a null to naettMake.
+		ERROR_LOG(Log::HTTP, "Couldn't create a request for '%s'", url_.c_str());
+		resultCode_ = naettGenericError;
+		failed_ = true;
+		completed_ = true;
+		sink_.reset();
+		progress_.Update(0, 0, true);
+		return;
+	}
 	res_ = naettMake(req_);
+	if (!res_) {
+		ERROR_LOG(Log::HTTP, "Couldn't start a request for '%s'", url_.c_str());
+		naettFree(req_);
+		req_ = nullptr;
+		resultCode_ = naettGenericError;
+		failed_ = true;
+		completed_ = true;
+		sink_.reset();
+		progress_.Update(0, 0, true);
+		return;
+	}
 
 	progress_.Update(0, 0, false);
 }
@@ -52,23 +124,37 @@ 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(std::move(sink_));
+		}
+		res_ = nullptr;
+		req_ = nullptr;
 	}
 }
 
 bool HTTPSRequest::Done() {
 	if (completed_)
 		return true;
-
-	_dbg_assert_(res_ != nullptr);
+	if (!res_) {
+		// Never started, or already let go of. Nothing left to wait for.
+		return true;
+	}
 
 	if (!naettComplete(res_)) {
 		int total = 0;
@@ -79,10 +165,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_ ? (int)sink_->buffer.size() : 0;
+	if (bodyLength > 0) {
+		buffer_.Append(sink_->buffer);
+	}
 	if (resultCode_ < 0) {
 		// It's a naett error. Translate and handle.
 		switch (resultCode_) {
@@ -101,6 +188,10 @@ bool HTTPSRequest::Done() {
 		case naettGenericError:  // -5
 			ERROR_LOG(Log::HTTP, "Generic error");
 			break;
+		case -1000:
+			// Ours, not naett's - see above. Cancelling is a normal thing to do, not an error.
+			INFO_LOG(Log::HTTP, "Request to '%s' cancelled", url_.c_str());
+			break;
 		default:
 			ERROR_LOG(Log::HTTP, "Unhandled naett error %d", resultCode_);
 			break;

Common/Net/HTTPNaettRequest.h

@@ -1,7 +1,8 @@
 #pragma once
 
-#include <thread>
+#include <memory>
 #include <string_view>
+#include <thread>
 
 #include "Common/Net/HTTPRequest.h"
 
@@ -11,6 +12,8 @@
 
 namespace http {
 
+struct NaettBodySink;
+
 // Really an asynchronous request.
 class HTTPSRequest : public Request {
 public:
@@ -24,12 +27,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 part 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, so ownership has to be able to move elsewhere. See NaettBodySink in the .cpp.
+	std::unique_ptr<NaettBodySink> sink_;
+
 	// Naett state
 	naettReq *req_ = nullptr;
 	naettRes *res_ = nullptr;

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.

Common/Net/Resolve.cpp

@@ -31,7 +31,6 @@ extern JavaVM *gJvm;
 
 namespace net {
 
-static bool g_naettInitialized;
 static bool g_wsaInitialized;
 
 void Init() {
@@ -44,19 +43,18 @@ void Init() {
 		g_wsaInitialized = true;
 	}
 #endif
-	if (!g_naettInitialized) {
+	// naett ignores repeat calls the same way WSAStartup does, so there's nothing to track here
+	// either. HTTPSAvailable is cheap to ask twice - on Linux it's a cached dlopen result.
 #ifndef HTTPS_NOT_AVAILABLE
 #if PPSSPP_PLATFORM(ANDROID)
-		_assert_(gJvm != nullptr);
-		naettInit(gJvm);
+	_assert_(gJvm != nullptr);
+	naettInit(gJvm);
 #else
-		if (HTTPSAvailable()) {
-			naettInit(NULL);
-		}
-#endif
-#endif
-		g_naettInitialized = true;
+	if (HTTPSAvailable()) {
+		naettInit(NULL);
 	}
+#endif
+#endif
 }
 
 bool HTTPSAvailable() {

ext/naett-lib/README-ppsspp.md

@@ -100,3 +100,22 @@ Keep this list up to date - it's what makes it possible to move to a newer upstr
 - `src/naett_osx.c`: `invalidateAndCancel` returns before the session lets go of its delegate, so
   the delegate's back pointer to the response is cleared first, and `didReceiveData` checks it
   (as `didCompleteWithError` already did).
+- `src/naett_osx.c` / `src/naett_android.c`: both threw away the body writer's return value.
+  Windows and Linux already treat a short write as a failed request - it's how the default
+  writer reports it couldn't grow, and how we cancel a transfer - so on those two a short write
+  silently truncated the body and still looked like a success. Apple also cancels the task, or
+  the data just keeps arriving.
+- `naett.h`: documented what the body writer's return value means, since three of the four
+  backends' behaviour depends on it.
+- `src/naett_win.c`: a short write from the body writer marked the request complete and then
+  queued another read anyway, so the transfer carried on and WinHTTP kept writing into a
+  response the caller was by then free to close. It stops there now, and the callback returns
+  early for anything raised after the request is complete.
+- `src/naett_core.c`: `naettMake` only asserted that the request wasn't NULL, and the request
+  constructors return NULL when the platform can't set one up - a URL it can't parse, say. In a
+  release build that walked into a null dereference. Returns NULL instead.
+- `src/naett_osx.c`: the delegate callbacks ignore a response that's already complete, so a
+  cancellation we asked for doesn't overwrite the error that caused it.
+- `src/naett_core.c` / `naett.h`: `naettInit` asserted it was only ever called once, so a caller
+  that can be entered more than again needed a flag purely to guard it. A repeat call is a no-op
+  now, the way `WSAStartup` behaves, and `net::Init` dropped its `g_naettInitialized`.

ext/naett-lib/naett.h

@@ -17,6 +17,10 @@ typedef void* naettInitData;
 /**
  * @brief Global init method.
  * Call to initialize the library.
+ *
+ * PPSSPP: calling this more than once is fine - everything after the first call does nothing,
+ * including the platform setup, so the init data from the first call is the one that sticks.
+ * Not thread safe; call it during startup, before anything else can reach the library.
  */
 void naettInit(naettInitData initThing);
 
@@ -41,6 +45,11 @@ naettOption* naettBody(const char* body, int size);
 // Sets a request body reader.
 naettOption* naettBodyReader(naettReadFunc reader, void* userData);
 // Sets a response body writer.
+//
+// PPSSPP: the writer is handed each chunk as it arrives, on whichever thread the backend runs
+// its transfer on, and must return the number of bytes it took. Returning anything else fails
+// the request with naettReadError and stops the transfer - which is both how the default writer
+// reports that it couldn't grow its buffer, and how a caller cancels one in progress.
 naettOption* naettBodyWriter(naettWriteFunc writer, void* userData);
 // Sets connection timeout in milliseconds.
 naettOption* naettTimeout(int milliSeconds);

ext/naett-lib/src/naett_android.c

@@ -297,7 +297,14 @@ static void* processRequest(void* data) {
             break;
         } else if (bytesRead > 0) {
             (*env)->GetByteArrayRegion(env, buffer, 0, bytesRead, (jbyte*) byteBuffer);
-            req->options.bodyWriter(byteBuffer, bytesRead, req->options.bodyWriterData);
+            // PPSSPP: a short write from the body writer fails the request - it's how a caller
+            // aborts, and how the default writer reports that it couldn't grow. Upstream ignored
+            // it, so the body was silently truncated and still reported as a success.
+            int written = req->options.bodyWriter(byteBuffer, bytesRead, req->options.bodyWriterData);
+            if (written != bytesRead) {
+                res->code = naettReadError;
+                goto finally;
+            }
             res->totalBytesRead += bytesRead;
         }
     } while (!res->closeRequested);

ext/naett-lib/src/naett_core.c

@@ -137,7 +137,12 @@ static void applyOptionParams(InternalRequest* req, InternalOption* option) {
 // Public API
 
 void naettInit(naettInitData initData) {
-    assert(!initialized);
+    // PPSSPP: upstream asserted that this was the first call. Callers that can be entered more
+    // than once then need a flag of their own purely to guard this, which is bookkeeping the
+    // library may as well do itself - so a repeat call is a no-op instead.
+    if (initialized) {
+        return;
+    }
     naettPlatformInit(initData);
     initialized = 1;
 }
@@ -310,6 +315,12 @@ naettReq* naettRequestWithOptions(const char* url, int numOptions, const naettOp
 naettRes* naettMake(naettReq* request) {
     assert(initialized);
     assert(request != NULL);
+    // PPSSPP: naettRequest* returns NULL when the platform can't set the request up - a URL it
+    // can't parse, most likely - and the assert above is compiled out in release, so this used to
+    // walk straight into a null dereference.
+    if (request == NULL) {
+        return NULL;
+    }
 
     InternalRequest* req = (InternalRequest*)request;
     naettAlloc(InternalResponse, res);

ext/naett-lib/src/naett_osx.c

@@ -100,7 +100,8 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) {
     object_getInstanceVariable(self, "response", (void**)&res);
     // PPSSPP: didComplete already checked this; this one didn't, and the delegate outlives the
     // response when a session is invalidated.
-    if (res == NULL) {
+    if (res == NULL || res->complete) {
+        // PPSSPP: once complete, the caller may already be closing this - don't touch it further.
         release(p);
         return;
     }
@@ -153,7 +154,18 @@ void didReceiveData(id self, SEL _sel, id session, id dataTask, id data) {
     const void* bytes = objc_msgSend_t(const void*)(data, sel("bytes"));
     NSUInteger length = objc_msgSend_t(NSUInteger)(data, sel("length"));
 
-    res->request->options.bodyWriter(bytes, (int)length, res->request->options.bodyWriterData);
+    // PPSSPP: a writer that doesn't take everything is failing the request - that's how a caller
+    // aborts a transfer, and how the default writer reports that it couldn't grow. Upstream threw
+    // the result away here, so a short write silently truncated the body and still looked like a
+    // success. Cancel the task too, or the data just keeps coming.
+    int written = res->request->options.bodyWriter(bytes, (int)length, res->request->options.bodyWriterData);
+    if (written != (int)length) {
+        res->code = naettReadError;
+        res->complete = 1;
+        objc_msgSend_void(dataTask, sel("cancel"));
+        release(p);
+        return;
+    }
     res->totalBytesRead += (int)length;
 
     release(p);
@@ -163,6 +175,11 @@ static void didComplete(id self, SEL _sel, id session, id dataTask, id error) {
     InternalResponse* res = NULL;
     object_getInstanceVariable(self, "response", (void**)&res);
     if (res != NULL) {
+        // PPSSPP: if we already failed the request - a writer that wouldn't take the data, say -
+        // that's the reason we want reported, not the cancellation it caused.
+        if (res->complete) {
+            return;
+        }
         if (error != nil) {
             res->code = naettConnectionError;
         }

ext/naett-lib/src/naett_win.c

@@ -111,6 +111,13 @@ static void CALLBACK
 callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInformation, DWORD statusInfoLength) {
     InternalResponse* res = (InternalResponse*)context;
 
+    // PPSSPP: once we've reported the request as complete the caller is free to close it, so
+    // don't start any more I/O against it - anything still in flight would be writing into a
+    // response that's being freed.
+    if (res->complete) {
+        return;
+    }
+
     switch (status) {
         case WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE: {
             // PPSSPP: the sizing call only fills in bufSize when it fails with
@@ -177,9 +184,17 @@ callback(HINTERNET request, DWORD_PTR context, DWORD status, LPVOID statusInform
             size_t bytesRead = statusInfoLength;
 
             InternalRequest* req = res->request;
+            if (req == NULL) {
+                return;
+            }
             if (req->options.bodyWriter(res->buffer, (int)bytesRead, req->options.bodyWriterData) != bytesRead) {
+                // PPSSPP: the writer failed the request - it couldn't grow, or the caller is
+                // cancelling. Upstream marked it complete and then queued another read anyway,
+                // which both kept the transfer running and left WinHTTP writing into a response
+                // the caller was by then free to close.
                 res->code = naettReadError;
                 res->complete = 1;
+                break;
             }
             res->totalBytesRead += (int)bytesRead;
             // PPSSPP: bytesLeft is unsigned, so a read longer than announced used to wrap it