New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a handful of Sendable warnings #1419

Merged

glbrntt merged 2 commits into grpc:1.7.1-async-await from glbrntt:gb-more-sendable-conformance

Jun 1, 2022

Collaborator

glbrntt commented May 30, 2022

Motivation:

We require all request/response types to be Sendable as they always cross a
thread boundary. Our various async sequences and async writers are conditionally
Sendable if their Element is Sendable, however, in practice they always will
be. We require them to be Sendable as they are arguments to async functions
(see SE-0338). For the same reason, the async server context (and by extension
the async server handler) must also be Sendable.

Modifications:

Require passthrough message/source sequence to have Sendable elements so
that they are unconditionally Sendable
Make the async server context and handler Sendable
Make the async writer error Sendable
Remove a global variable from protoc-gen-grpc-swift
Add appropriate conditions to extensions on Call which define functions
requiring Call to be Sendable

Result:

Better Sendable support.


          Fix a handful of Sendable warnings

9d9aeac

Motivation:

We require all request/response types to be `Sendable` as they always cross a
thread boundary. Our various async sequences and async writers are conditionally
Sendable if their `Element` is `Sendable`, however, in practice they always will
be. We require them to be `Sendable` as they are arguments to `async` functions
(see SE-0338). For the same reason, the async server context (and by extension
the async server handler) must also be `Sendable`.

Modifications:

- Require passthrough message/source sequence to have `Sendable` elements so
  that they are unconditionally `Sendable`
- Make the async server context and handler `Sendable`
- Make the async writer error Sendable
- Remove a global variable from protoc-gen-grpc-swift
- Add appropriate conditions to extensions on `Call` which define functions
  requiring `Call` to be `Sendable`

Result:

Better `Sendable` support.

glbrntt requested a review from fabianfett

May 30, 2022 14:54

FranzBusch approved these changes

View reviewed changes

fabianfett reviewed

View reviewed changes

Sources/GRPC/AsyncAwaitSupport/AsyncWriter.swift Outdated

@@ @@ -296,11 +296,11 @@ extension AsyncWriter where End == Void { @@
                 }
               }
-              public struct GRPCAsyncWriterError: Error, Hashable {
+              public struct GRPCAsyncWriterError: Error, Sendable, Hashable {

Collaborator

fabianfett May 30, 2022

Sendability is already required from Error conformance.

Contributor

Davidde94 May 30, 2022

https://developer.apple.com/documentation/swift/error

fabianfett requested changes

View reviewed changes

Collaborator

fabianfett left a comment

I still think we don't need to require Request and Response to be Sendable.

Sources/GRPC/AsyncAwaitSupport/GRPCAsyncResponseStream.swift

@@ @@ -18,7 +18,7 @@ @@
               /// This is currently a wrapper around AsyncThrowingStream because we want to be
               /// able to swap out the implementation for something else in the future.
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
-              public struct GRPCAsyncResponseStream<Element>: AsyncSequence {
+              public struct GRPCAsyncResponseStream<Element: Sendable>: AsyncSequence {

Collaborator

fabianfett May 30, 2022

I don't think, we should enforce this.

Collaborator

Lukasa May 31, 2022

I think we have to. The underlying elements are produced from a NIO thread, so they are necessarily Sendable. The type is useless without that.

Sources/GRPC/AsyncAwaitSupport/Call+AsyncRequestStreamWriter.swift

@@ @@ -16,7 +16,7 @@ @@
               #if compiler(>=5.6)
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
-              extension Call {
+              extension Call where Request: Sendable, Response: Sendable {

Collaborator

fabianfett May 30, 2022

I don't think, we should enforce this.

Contributor

Davidde94 May 30, 2022

Conditional extensions aren't enforcements - if Response and a Request don't conform then neither will this

Collaborator Author

glbrntt May 31, 2022

As GRPCAsyncRequestStreamWriter expects Request to be Sendable we require it be here. We also require Response to be Sendable to that Call is Sendable which we require to capture it in the Sendable closures for the delegate.

Sources/GRPC/AsyncAwaitSupport/GRPCAsyncServerHandler.swift

@@ @@ -798,7 +803,7 @@ protocol AsyncServerCallContextProvider { @@
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
               @usableFromInline
-              internal struct ServerHandlerComponents<Request, Delegate: AsyncWriterDelegate> {
+              internal struct ServerHandlerComponents<Request: Sendable, Delegate: AsyncWriterDelegate> {

Collaborator

fabianfett May 30, 2022

I don't think, we should enforce this.

Collaborator Author

glbrntt May 31, 2022

We need this one to enforce it for the PassthroughMessageSource

Sources/GRPC/AsyncAwaitSupport/PassthroughMessageSequence.swift

@@ @@ -18,7 +18,7 @@ @@
               /// An ``AsyncSequence`` adapter for a ``PassthroughMessageSource``.`
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
               @usableFromInline
-              internal struct PassthroughMessageSequence<Element, Failure: Error>: AsyncSequence {
+              internal struct PassthroughMessageSequence<Element: Sendable, Failure: Error>: AsyncSequence {

Collaborator

fabianfett May 30, 2022

I don't think, we should enforce this.

Sources/GRPC/AsyncAwaitSupport/PassthroughMessageSource.swift

@@ @@ -29,7 +29,7 @@ import NIOCore @@
               /// indicate that the sequence should end with an error.
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
               @usableFromInline
-              internal final class PassthroughMessageSource<Element, Failure: Error> {
+              internal final class PassthroughMessageSource<Element: Sendable, Failure: Error> {

Collaborator

fabianfett May 30, 2022

I don't think, we should enforce this.

Collaborator

Lukasa May 31, 2022

I'm curious as to why not. A non-Sendable AsyncSequence is next to useless: you can only send to it and receive from it on the same Task. When is anyone ever going to do that?


          Remove redundant Sendable

a3f666b

glbrntt commented

View reviewed changes

Collaborator Author

glbrntt left a comment

We talked a little offline about whether or not the various AsyncSequences should be required to be Sendable here and whether Request and Response are required to be Sendable.

The summary is that Request and Response must always be Sendable: they are always transferred across a thread boundary (event loop to concurrency context). For async sequences in some cases it would be useful to be able to move the ownership of the AsyncSequence to another concurrency context (in particular moving it from the event-loop context to a concurrency context). However we can't currently represent that and so we must make them Sendable.

Sources/GRPC/AsyncAwaitSupport/Call+AsyncRequestStreamWriter.swift

@@ @@ -16,7 +16,7 @@ @@
               #if compiler(>=5.6)
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
-              extension Call {
+              extension Call where Request: Sendable, Response: Sendable {

Collaborator Author

glbrntt May 31, 2022

As GRPCAsyncRequestStreamWriter expects Request to be Sendable we require it be here. We also require Response to be Sendable to that Call is Sendable which we require to capture it in the Sendable closures for the delegate.

Sources/GRPC/AsyncAwaitSupport/GRPCAsyncServerHandler.swift

@@ @@ -798,7 +803,7 @@ protocol AsyncServerCallContextProvider { @@
               @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
               @usableFromInline
-              internal struct ServerHandlerComponents<Request, Delegate: AsyncWriterDelegate> {
+              internal struct ServerHandlerComponents<Request: Sendable, Delegate: AsyncWriterDelegate> {

Collaborator Author

glbrntt May 31, 2022

We need this one to enforce it for the PassthroughMessageSource

glbrntt requested a review from fabianfett

May 31, 2022 12:13

fabianfett approved these changes

View reviewed changes

Collaborator

fabianfett left a comment

Thanks for the discussion. I like all the changes here, but wish Swift would have gone the send and sync way.

glbrntt merged commit 3426bca into grpc:1.7.1-async-await

glbrntt deleted the gb-more-sendable-conformance branch

June 1, 2022 14:41

glbrntt added the 🔨 semver/patch label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch