Support cancellation of tasks with FetchHTTPClient

[xbhatnag]

The client now supports cancellation during fetch() API call or during reading of the response body!

The example app is now configured to cancel a request after 10 seconds to demonstrate this functionality.

[guoye-zhang]

Are these functions safe to call from any threads?

[xbhatnag]

I'm no expert on how this translates to JS, but isn't JS single-threaded, making WASM single-threaded, making all the asynchronous contexts in Swift also single-threaded?

[guoye-zhang]

I believe that multithreading is supported in WASM: https://webassembly.org/features/

[xbhatnag]

Today I learned :) @MaxDesiatov might know more about this specific question...

[MaxDesiatov]

Our Swift SDK for Wasm is single-threaded by default. Multi-threading is available as an option, so I don't think @unchecked Sendable is justifiable here.

[kateinoigakukun]

Any JS methods against a JSObject should be called on the thread on which the JSObject is created. So as long as those methods are called on the thread the AbortController instance is created on, it's safe. However if we cannot guarantee that, we need to use JSRemote<T> to call them safely: swiftwasm/JavaScriptKit#711

[guoye-zhang]

How does JS thread map to Swift concurrency? There is no guarantee of the thread that an async method runs on since threads can switch arbitrarily after an await right? Should we wrap everything in JSRemote or just use @MainActor? (I assume that MainActor maps to the main thread in JS as well)

[kateinoigakukun]

MainActor-isolated code is consistently mapped to the browser main thread in any case, and there is no guarantee that any other async code runs on. However, practically speaking, JavaScriptKit does not provide a global concurrent executor even under a multithreading environment, and it just provides TaskExecutor implementations, and those implementations guarantee no thread hop in a structured async code so that client code can control which thread a specific code can run on in an opt-in way.

I took a closer look on the code, and I can say FetchHTTPClient.perform itself is safe because all JS object access happens within a structured code, but ResponseBodyReader.read has to use JSRemote because the caller might be running on a different thread than the thread FetchHTTPClient.perform was called.

After swiftwasm/JavaScriptKit#747 lands, we can fix the potential thread issue by the following patch.

Patch

diff --git a/Sources/FetchHTTPClient/FetchHTTPClient.swift b/Sources/FetchHTTPClient/FetchHTTPClient.swift
index cb6825e..d5c4bc4 100644
--- a/Sources/FetchHTTPClient/FetchHTTPClient.swift
+++ b/Sources/FetchHTTPClient/FetchHTTPClient.swift
@@ -120,7 +120,7 @@ public final class FetchHTTPClient: HTTPAPIs.HTTPClient {

         return try await responseHandler(
             HTTPResponse(status: .init(code: responseStatus, reasonPhrase: responseStatusText), headerFields: responseHeaders),
-            ResponseReader(reader: reader)
+            ResponseReader(reader: JSRemote(reader))
         )
     }

@@ -145,7 +145,7 @@ public final class FetchHTTPClient: HTTPAPIs.HTTPClient {
     }

     public struct ResponseReader: ConcludingAsyncReader, ~Copyable {
-        let reader: ReadableStreamDefaultReader
+        let reader: JSRemote<ReadableStreamDefaultReader>

         public consuming func consumeAndConclude<Return, Failure>(
             body: nonisolated(nonsending) (consuming sending FetchHTTPClient.ResponseBodyReader) async throws(Failure) -> Return
@@ -159,7 +159,7 @@ public final class FetchHTTPClient: HTTPAPIs.HTTPClient {
         public typealias ReadFailure = any Error
         public typealias Buffer = UniqueArray<UInt8>

-        let reader: ReadableStreamDefaultReader
+        let reader: JSRemote<ReadableStreamDefaultReader>
         var buffer = UniqueArray<UInt8>()

         public mutating func read<Return: ~Copyable, Failure>(
@@ -167,26 +167,35 @@ public final class FetchHTTPClient: HTTPAPIs.HTTPClient {
+        let reader: JSRemote<ReadableStreamDefaultReader>
         var buffer = UniqueArray<UInt8>()

         public mutating func read<Return: ~Copyable, Failure>(
@@ -167,26 +167,35 @@ public final class FetchHTTPClient: HTTPAPIs.HTTPClient {
         ) async throws(AsyncStreaming.EitherError<any Error, Failure>) -> Return where Failure: Error {
             let chunk: Chunk
             do {
-                chunk = try await self.reader.read()
+                chunk = try await reader.withJSObject { reader in
+                    try await reader.read()
+                }
+            } catch {
+                throw .first(error)
+            }
+            do {
+                try self.append(chunk: chunk)
             } catch {
                 throw .first(error)
             }
+            do {
+                return try await body(&self.buffer)
+            } catch {
+                throw .second(error)
+            }
+        }
+
+        private mutating func append(chunk: Chunk) throws {
             if !chunk.done {
                 guard let bytes = chunk.value, !bytes.isEmpty else {
                     // If not done, there must be bytes that can be read
-                    throw .first(FetchError.BadAssumptionJS)
+                    throw FetchError.BadAssumptionJS
                 }
                 buffer.reserveCapacity(bytes.count)
                 for b in bytes {
                     self.buffer.append(b)
                 }
             }
-
-            do {
-                return try await body(&self.buffer)
-            } catch {
-                throw .second(error)
-            }
         }
     }
 }

[guoye-zhang]

Hope to understand a bit more how this works. Is fetch allowed to be called from any thread? Does JSRemote internally switch back to the creating thread to call methods?

Wondering if there is an efficiency difference between wrapping the reader in JSRemote or just slapping MainActor on everything to let Swift handle thread hopping.

[MaxDesiatov]

Can we get around by making it non-Sendable in this PR (@MainActor could even maybe be acceptable?), and then following up with JSRemote approach in a future PR when upstream JavaScriptKit has everything ready for that?

[MaxDesiatov]

Ditto, @unchecked Sendable is most probably not correct here.

cc @kateinoigakukun