Make clients sendable #1386

glbrntt · 2022-04-05T13:29:27Z

Motivation:

gRPC clients should be Sendable.

Modifications:

This commit contains a number of changes to enable clients to be
'Sendable'. The are:

Require GRPCChannel to be 'Sendable' since the underlying API should
be thread-safe.
Require GRPCClient to be 'Sendable' as this just provides wrappers
around a GRPCChannel which is also thread-safe. This propagates to
generated client protocols which refine GRPCClient.
Generated clients using the NIO API are classes (!) holding mutable
call options and interceptor factories. Codegen was updated so make
these @unchecked Sendable by protecting mutable state with a lock.
Because this generates additional allocations and should never have
been a class it is now marked deprecated in favor of a struct based
client.
AnyServiceClient is also a class and has been deprecated in favor of
the struct based GRPCAnyServiceClient.
A separate commit fixes these deprecation warnings.
The test clients have also been marked as deprecated: they rely on an
EmbeddedChannel which is not and will never be Sendable (without
also being deprecated). The test clients are also marked
as @unchecked Sendable to satisfy the requirement on GRPCClient
despite being deprecated. The same is true for the FakeChannel (a
GRPCChannel). We reccomend using a client and server on localhost as
a replacement.
Various supporting value types are marked as Sendable.

Result:

Clients are now Sendable.
Clients are now always structs.
Generated test clients are deprecated.

glbrntt · 2022-04-05T13:30:26Z

Note to reviewers:

The bulk of the changes are in: 5500255
Generated clients are updated in: 409b285
Deprecation warnings are fixed in: 14e3b26

glbrntt · 2022-04-05T13:32:33Z

Sources/GRPC/GRPCChannel/EmbeddedGRPCChannel.swift

@@ -1,113 +0,0 @@
-/*


This file was only used in tests, its new home is in Tests/GRPCTests/ClientTimeoutTests.swift.

glbrntt · 2022-04-05T13:33:13Z

Tests/GRPCTests/ClientTimeoutTests.swift

+private final class EmbeddedGRPCChannel: GRPCChannel {
+  let embeddedChannel: EmbeddedChannel
+  let multiplexer: EventLoopFuture<HTTP2StreamMultiplexer>
+
+  let logger: Logger
+  let scheme: String
+  let authority: String
+  let errorDelegate: ClientErrorDelegate?
+
+  func close() -> EventLoopFuture<Void> {
+    return self.embeddedChannel.close()
+  }
+
+  var eventLoop: EventLoop {
+    return self.embeddedChannel.eventLoop
+  }
+
+  init(
+    logger: Logger = Logger(label: "io.grpc", factory: { _ in SwiftLogNoOpLogHandler() }),
+    errorDelegate: ClientErrorDelegate? = nil
+  ) {
+    let embeddedChannel = EmbeddedChannel()
+    self.embeddedChannel = embeddedChannel
+    self.logger = logger
+    self.multiplexer = embeddedChannel.configureGRPCClient(
+      errorDelegate: errorDelegate,
+      logger: logger
+    ).flatMap {
+      embeddedChannel.pipeline.handler(type: HTTP2StreamMultiplexer.self)
+    }
+    self.scheme = "http"
+    self.authority = "localhost"
+    self.errorDelegate = errorDelegate
+  }
+
+  internal func makeCall<Request: Message, Response: Message>(
+    path: String,
+    type: GRPCCallType,
+    callOptions: CallOptions,
+    interceptors: [ClientInterceptor<Request, Response>]
+  ) -> Call<Request, Response> {
+    return Call(
+      path: path,
+      type: type,
+      eventLoop: self.eventLoop,
+      options: callOptions,
+      interceptors: interceptors,
+      transportFactory: .http2(
+        channel: self.makeStreamChannel(),
+        authority: self.authority,
+        scheme: self.scheme,
+        // This is internal and only for testing, so max is fine here.
+        maximumReceiveMessageLength: .max,
+        errorDelegate: self.errorDelegate
+      )
+    )
+  }
+
+  internal func makeCall<Request: GRPCPayload, Response: GRPCPayload>(
+    path: String,
+    type: GRPCCallType,
+    callOptions: CallOptions,
+    interceptors: [ClientInterceptor<Request, Response>]
+  ) -> Call<Request, Response> {
+    return Call(
+      path: path,
+      type: type,
+      eventLoop: self.eventLoop,
+      options: callOptions,
+      interceptors: interceptors,
+      transportFactory: .http2(
+        channel: self.makeStreamChannel(),
+        authority: self.authority,
+        scheme: self.scheme,
+        // This is internal and only for testing, so max is fine here.
+        maximumReceiveMessageLength: .max,
+        errorDelegate: self.errorDelegate
+      )
+    )
+  }
+
+  private func makeStreamChannel() -> EventLoopFuture<Channel> {
+    let promise = self.eventLoop.makePromise(of: Channel.self)
+    self.multiplexer.whenSuccess {
+      $0.createStreamChannel(promise: promise) {
+        $0.eventLoop.makeSucceededVoidFuture()
+      }
+    }
+    return promise.futureResult
+  }
+}


This was moved from Sources/GRPC/GRPCChannel/EmbeddedGRPCChannel.swift.

Motivation: gRPC clients should be 'Sendable'. Modifications: This commit contains a number of changes to enable clients to be 'Sendable'. The are: - Require `GRPCChannel` to be 'Sendable' since the underlying API should be thread-safe. - Require `GRPCClient` to be 'Sendable' as this just provides wrappers around a `GRPCChannel` which is also thread-safe. This propagates to generated client protocols which refine `GRPCClient`. - Generated clients using the NIO API are classes (!) holding mutable call options and interceptor factories. Codegen was updated so make these `@unchecked Sendable` by protecting mutable state with a lock. Because this generates additional allocations and should never have been a class it is now marked deprecated in favor of a struct based client. - `AnyServiceClient` is also a class and has been deprecated in favor of the struct based `GRPCAnyServiceClient`. - A separate commit fixes these deprecation warnings. - The test clients have also been marked as deprecated: they rely on an `EmbeddedChannel` which is not and will never be `Sendable` (without also being deprecated). The test clients are also marked as `@unchecked Sendable` to satisfy the requirement on `GRPCClient` despite being deprecated. The same is true for the `FakeChannel` (a `GRPCChannel`). We reccomend using a client and server on localhost as a replacement. - Various supporting value types are marked as `Sendable`. Result: - Clients are now `Sendable`. - Clients are now always `struct`s. - Generated test clients are deprecated.

Lukasa · 2022-04-05T14:46:52Z

Sources/GRPC/CallOptions.swift

@@ -160,14 +174,22 @@ extension CallOptions {
    }

    /// Provide a factory to generate request IDs.
+    #if swift(>=5.6)
+    public static func generated(
+      _ requestIDFactory: @escaping @Sendable () -> String


Should we promote the StringFactory typealias to public here to avoid the extra #if?

Lukasa · 2022-04-05T14:52:19Z

Sources/GRPC/Interceptor/ClientInterceptors.swift

+// Interceptors sendability is unchecked: they have their own documented rules for thread safety
+// which we can't check. We require them to be sendable in order for interceptor factories to be
+// sendable and in turn, clients.
+extension ClientInterceptor: @unchecked Sendable {}


Does this need to be preconcurrency, given that the class already existed and is open for subclassing?

Ah, yes it sure does!

Actually I'm no so sure: we'd need to mark the class with @preconcurrency and doing so means we'd need to duplicate the whole class.

Also if we are to require it be Sendable then we must mark the base class as @unchecked Sendable because it's open (if we mark it as GRPCSendable we get "Non-final class 'ClientInterceptor' cannot conform to 'Sendable'; use '@unchecked Sendable'", which makes sense, we don't know the concrete type at compile time).

I think it'd have to be both @preconcurrency and @unchecked. But I think it's API breaking if we don't add the @preconcurrency annotation, because any adopters who are not concurrency aware but build in 5.6 mode will break.

I think they inherit the @unchecked (none of the subclasses within GRPCTests complained about not being sendable, some of which have mutable state)?

I don't think @unchecked should be inherited, so far as I know.

However, I can't find any immediate citation for that belief.

Hmmm, this seems relevant.

As does the final post https://forums.swift.org/t/improving-sendable/52377/26:

There are a number of unresolved issues with Sendable that could be tackled separately. Here's the list I know of:
...

Sendable with class types needs to be revisited. We likely want to require subclasses to restate Sendable conformance, decide if there are any cases where that conformance need not be @unchecked (e.g., all state is immutable and Sendable) and whether it can ever be inferred (same conditions as for removing @unchecked?).

I'm not sure exactly where that leaves us but I think @preconcurrency and @unchecked is probably the safest bet here.

glbrntt · 2022-04-05T15:48:35Z

Also not sure if ...NIOClient is the right new spelling for the future/callback based API (the client with the async based API is ...AsyncClient). Happy to take suggestions.

Lukasa · 2022-04-05T15:52:36Z

I think NIOClient is totally fine there.

glbrntt commented Apr 5, 2022

View reviewed changes

glbrntt added 3 commits April 5, 2022 15:19

Regenerate

409b285

Fix deprecation warnings

14e3b26

glbrntt force-pushed the gb-more-sendable branch from c6599ea to 14e3b26 Compare April 5, 2022 14:19

Lukasa reviewed Apr 5, 2022

View reviewed changes

Use a typealias

6dca9ce

glbrntt marked this pull request as ready for review April 5, 2022 15:26

glbrntt added the 🆕 semver/minor Adds new public API. label Apr 5, 2022

Client interceptors are preconcurrency and unchecked sendable

ae67e3e

glbrntt requested a review from Lukasa April 11, 2022 08:34

Lukasa approved these changes Apr 11, 2022

View reviewed changes

Lukasa merged commit 305677e into grpc:1.7.1-async-await Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make clients sendable #1386

Make clients sendable #1386

glbrntt commented Apr 5, 2022

glbrntt commented Apr 5, 2022 •

edited

Loading

glbrntt Apr 5, 2022

glbrntt Apr 5, 2022

Lukasa Apr 5, 2022

Lukasa Apr 5, 2022

glbrntt Apr 5, 2022

glbrntt Apr 5, 2022

Lukasa Apr 5, 2022

glbrntt Apr 5, 2022

Lukasa Apr 5, 2022

Lukasa Apr 5, 2022

Lukasa Apr 5, 2022

glbrntt Apr 5, 2022

glbrntt commented Apr 5, 2022

Lukasa commented Apr 5, 2022

Make clients sendable #1386

Make clients sendable #1386

Conversation

glbrntt commented Apr 5, 2022

glbrntt commented Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt commented Apr 5, 2022

Lukasa commented Apr 5, 2022

glbrntt commented Apr 5, 2022 •

edited

Loading