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.

[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?

[FranzBusch]

I don't think we can AbortController is being called in the onCancel closure which can be called from an arbitrary thread. In general, we cannot make any guarantees of threads in Swift Concurrency.

So I think this comes down to the concurrency threads that are used by the executors here. Not sure what JavaScriptEventLoop does here but it might be better that those threads provide access to abort controllers instead of creating them. I haven't deeply thought about this though.

[MaxDesiatov]

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

cc @kateinoigakukun

[FranzBusch]

Can we update the branch against latest main at least and resolve the open threading question.

[FranzBusch]

Can we re-write this example to avoid top level code and use @main instead. Then we avoid the unstructured tasks as well.

[FranzBusch]

I don't think we can AbortController is being called in the onCancel closure which can be called from an arbitrary thread. In general, we cannot make any guarantees of threads in Swift Concurrency.

So I think this comes down to the concurrency threads that are used by the executors here. Not sure what JavaScriptEventLoop does here but it might be better that those threads provide access to abort controllers instead of creating them. I haven't deeply thought about this though.