Merge the async branch into main #1428
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: To test async/await, we actually need a Swift version in CI that supports it.
This PR implements some of the types required by the proposal for async/await support, added in grpc#1231. To aid reviewing, only the types required for the client are included. They have been pulled in from the proof-of-concept implementation linked from the proposal PR. Note that this makes use of a clunky mechanism for yielding results into the response stream. @glbrntt is working on a parallel effort to provide a more flexible implementation of `AsyncSequence` that allows for values to be yielded from outside of the closure parameter to the initializer. When it is ready, this code will be updated to make use of it. There is a note in the code comments to this effect.
Motivation: `AsyncThrowingStream` provides an implementation of `AsyncSequence` which allows the holder to provide new values to that sequence from within a closure provided to the initializer. This API doesn't fit our needs: we must be able to provide the values 'from the outside' rather than during initialization. Modifications: - Add a `PassthroughMessageSequence`, an implementation of `AsyncSequence` which consumes messages from a `PassthroughMessagesSource`. - The source may have values provided to it via `yield(_:)` and terminated with `finish()` or `finish(throwing:)`. - Add tests and a few `AsyncSequence` helpers. Result: We have an `AsyncSequence` implementation which can have values provided to it.
Motivation: Async RPCs which stream outbound messages (i.e. requests on the client, responses on the server) will be provided with a means to asynchronously send messages. This object should suspend if the underlying channel is not currently writable. Modifications: Add an 'AsyncWriter' as the underlying type for these objects which suspends if the writer is 'paused' and resumes when the writer is resumed. Result: We have a writer capable of suspending writes.
This commit implements some of the types required by the proposal for async/await support, added in grpc#1231. To aid reviewing, only the types required for the server are included. They have been pulled in from the proof-of-concept implementation linked from the proposal PR. It is a complimentary PR to grpc#1243 ("Async-await: Base types for client implementation"). It provides a unified `AsyncServerHandler` for all of the RPC types which avoids a substantial amount of code duplication that is found in the existing handlers. Wrappers are provided for the four RPC types. Otherwise it is analogous to the existing `BidirectionalStreamingServerHandler`. It's worth calling out that this PR makes use of some placeholder types which are not intended to be final. Specifically: * `AsyncResponseStreamWriter` is expected to be superseded by the `AsyncWriter` from grpc#1245. * `AsyncServerCallContext` conformance has been added to the existing `ServerCallContextBase`. It is expected that we will provide a new implementation of `AsyncServerCallContext` that is independent from the existing call context types.
Motivation: Manually constructing clients and servers is an error prone nightmare. We should generate them instead! Modifications: - Add async-await code-generation for server and client. - The client code generation is missing "simple-safe" wrappers for now, this can be added later. - Naming represents the current state of the branch rather than anything final - Add options for "ExperimentalAsyncClient" and "ExperimentalAsyncServer" -- these may be used in conjunction with the 'regular' "Client" and "Server" options. Result: We can generate async-await style grpc clients and servers.
…n and basic tests (grpc#1260) Motivation: In grpc#1259 code generation was added for async client and servers. We should generate the echo service and have tests exercising the generated async code. Modifications: Switch on async code generation for echo Implement an async echo service and add some basic tests Results: We run tests with an async client and server
…1262) The adopter may wish to set the response headers (aka "initial metadata") from their user handler. Until now this was not possible, even in the existing non–async-await API, and it was only possible for an adopter to set the trailers. This introduces a new mutable property to the context passed to the user handler that allows them to set the headers that should be sent back to the client before the first response message. * `let GRPCAsyncServerCallContext.headers` has been renamed to `requestHeaders` to disambiguate from newly introduced property. * `var GRPCAsyncServerCallContext.trailers` has been renamed to `responseTrailers` to better align with newly introduced property. * `var GRPCAsyncServerCallContext.responseHeaders` has been introduced.
…1264) Motivation: The client code for async-await has some placeholder implementations for receiving responses and sending requests. Now that we have the necessary types we can replace the placeholders. Modifications: - Replace `AsyncStream` usage with a `PassthroughMessageSequence` - Add a `GRPCAsyncRequestStreamWriter` which request streaming RPCs may use to write requests and finish the request stream. - This writer depends on a delegate which uses callbacks to send messages on the underlying `Call`. - Request streaming RPCs now have a `requestStream` on which to send requests. - Wrap up `AsyncWriterError` as a `public` `struct`. Result: Clients may send requests on a `requestStream`.
Motivation: The 'right' way to check if Swift concurrency is available is to use `#if compiler(>=5.5) && canImport(_Concurrency)`. We are missing currently missing the `canImport` check. Modifications: Add check for `canImport(_Concurrency)`. Results: We more correctly check whether concurrency is available.
Motivation: We plan on tagging a release soon. Modifications: - Bump the version to 1.4.1-async-await.2 Result: The version in the default user-agent string will match the released version.
…1302) As reported in grpc#1299 and grpc#1301, there are some scenarios where the closing the channel or using the response stream writer after the channel has been closed will lead to a crash. Specifically, when `finish()` is called the state was not progressed to `.completed` before cancelling the task. This was to maintain parity with the ELG-based API where the status and the trailers were still sent after `finish()` is called. We now believe this to be misguided and we shouldn't expect to be able to send anything on the channel at this point because we are tearing the handler and the channel down. This changes `finish()` to move to the `.completed` state before cancelling the `userHandlerTask`. As a result, when the completion handler for the user function fires, it will call `handleError(_:)` with `CancellationError` (as before) but now the error handler will not attempt to send the status or trailers back via the interceptors because the state will be in `.completed`. Tests for receiving an error after headers and after a message have been added.
The interoperability tests exercise a well-defined sequence of requests for all implementations of gRPC. We already had interop tests for the ELG-based client and provider. This pull request extends the tests to exercise an async-await–based provider. Signed-off-by: Si Beaumont <beaumont@apple.com>
Motivation: Some of the new async-await APIs are inconsistent in their use of 'requests' vs 'requestStream' and 'responses' vs 'responseStream'. We should name things consistently. Modifications: - Rename 'requests' to 'requestStream' in generated server code - Rename 'responses' to 'responseStream' in client calls Result: More consistent naming.
Motivation: Sometimes it's useful to know information about a service, including its name, methods it offers and so on. An example where this is useful is service discovery (grpc#1183). However, we currently only provide the service name and this isn't available statically. For service discovery this is problematic as it requires users to create a client in order to get the service name so that a server can be dialled. For the async/await code, since it is not yet final, we can add requirements to generated protocols to provide the service metadata statically. Modifications: - Add service and method descriptors to `GRPC` - Generate service descriptors for client and server separately (it's feasible that a user may generate client code into one module and server code into separate modules) - Update the generated code for async/await and NIO based APIs to use the descriptors directly rather than generating literals in places where they are required. - Add test for the generated echo service metadata - Regenerate other services Result: Adopters can get static information about services.
Motivation: The async server handler throws an error if the end-of-stream received from the client is dropped. The end-of-stream will only be dropped if the request stream has already been finished. This happens in a few cases: either end-of-stream has already been received, or as a side effect of the task running the user handler completing. The latter case is possible of the user handler does not wait for end of stream (if it encounters an error, for example). This happened periodically in some tests as a result of the user handler completing and receiving end-stream racing. Modifications: - Tolerate dropping end stream in the async server handler. Result: Fewer flakey tests.
Motivation: Our examples should be up-to-date. The RouteGuide example currently uses the NIO event-loop-future API, it should use the async/await API instead. Modifications: - Convert the route guide example to async/await - Update the corresponding tutorial Result: RouteGuide is more up-to-date
Motivation: When the idle handler determines that the channel needs to be closed (becuase the connection is no longer required) it does so on the current event loop tick. Closing immediately means that some events which are already scheduled to run on the current tick may be dropped unexpectedly. Modifications: - Execute the channel close on the next event-loop tick. - Fixup a test which now requires an extra loop `run()`. Result: Shutdown is a little more graceful. (cherry picked from commit c43be58)
Motivation: Swift 5.5.2 includes XCTest support for async/await. Currently we are working around the lack of support with XCTest expectations. Modifications: - Remove the helper - Make async tests `async throws` Result: Fewer workarounds.
Motivation: Concurrency support was backported to older OS versions in Swift 5.5.2; we should lower our availability guards accordingly. Modifications: - Update compiler guards and availability in GRPC - Update generated guards Result: Async gRPC is availabile on older OSs.
Motivation: Generated 'makeMethodCall' functions on the async client use the casing of 'Method' as it is defined in the .proto file. Methods are defined in the proto file with 'UpperCamel' casing, so 'makeUpperCamelCall' is correctly cased. However a 'lowerCamel' method would be generated as 'makelowerCamelCall'. We should instead coerce it to be 'makeLowerCamelCall'. We missed this in other areas too, as an example, for interceptors we will generate 'makelowerCamelCallInterceptors'. Unfortunately we can't change this one without the risk of breaking adopters. However, since async is yet to be stable API we can fix this one. Modifications: - Use upper camel casing for 'makeMethodCall' function names Result: Better naming.
Motivation: swift-protobuf added `Sendable` conformance to generated messages in the 1.19.0 release. Modifications: - Bump minimum protobuf version - Regenerate examples and interop tests Result: Protobuf messages are `Sendable`
Motivation: The latest 5.5 Swift compiler is not too pleased with some of the `Sendable` annotations which causes the compiler to crash. This is fixed in 5.6. Given we haven't released this to 'main' yet and Swift 5.6 has been around for a little while we should just raise the minimum Swift version which supports async/await. Modifications: - Raise the minimum Swift version for async/await to 5.6. Result: Compiler is happier.
Motivation: The core async/await API should have appropriate 'Sendable' conformance before we publish it to main. This change adds it to the core parts of the new API. There are still numerous types which can be made 'Sendable' but are either less significant (e.g. configuration) or require depdendencies to adopt 'Sendable' first (and would otherwise require `@preconcurrency`) Modifications: - Add `GRPCSendable` to ease adoption - Make some plain-old-data types `Sendable` including `GRPCStatus` - Require `Request` and `Response` to be `Sendable` in the client and server async APIs - Make the async server context `@unchecked Sendable` - Make the async writer and stream reader `Sendable` Result: Core async API is `Sendable`.
* Update formatter Motivation: We need to use a newer version of SwiftFormat on the async/await code. First we should update main so that merging changes into the async branch is cleaner. Modifications: - Update SwiftFormat - Reformat Result: Our version of SwiftFormat is new enough to correctly handle async/await code. (cherry picked from commit 80055e5) * Reformat async code Motivation: On 'main' we updated SwiftFormat to a version which knows about language features introduced in 5.5 and formatted the code accordingly. We need to do the same on the async branch. Modifications: - Remove rules to temporarily disable SwiftFormat (in place for compatibility reasons) - Rerun the formatter Result: Formatter is happy.
* Always generate async code Motivation: Async code generation currently requires opting in. We should make it the default alongside the NIO-based code so that it's easier to find. Modifications: - Remove the async options from the generator. - Update the generator to always emit async code behind compiler guards. - Update plugin docs. - Regenerate code. Result: Async code is easier to find. * Regenerate
* Make clients sendable 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. * Regenerate * Fix deprecation warnings * Use a typealias * Client interceptors are preconcurrency and unchecked sendable
Motivation: https://bugs.swift.org/browse/SR-15955 precluded tests from running on Swift 5.6. This has been resolved in Swift 5.6.1. Modifications: - Re-enable tests on 5.6 - Disable TSAN on 5.6 (the tests for AsyncServerHandler are abusing `EmbeddedChannel`; the tests will be rewritten when the handler state machine is rewritten and the AsyncEmbeddedChannel comes to fruition) Result: Tests run on 5.6
Motivation: The existing async server handler has a state machine which deals with the interceptor pipeline and the lifecycle of user handler. These are really two separate entities each with its own state machine. This PR is the first in a series which will eventually update the async server handler to make use of the two state machines. Modifications: - Add a 'ServerInterceptorStateMachine' and tests. This acts as a filter for inbound and outbound request and response messages, both as they enter and exit the interceptor pipeline. Message parts delievered in the wrong order lead to the RPC being cancelled, parts delievered after the RPC has finished lead to them being dropped. Cancelling the RPC informs the caller to nil out the associated interceptor pipeline. - Tests - This is not used anywhere (yet). Result: Interceptor state machine is in place.
Motivation: See grpc#1394. Modifications: - Add a 'ServerHandlerStateMachine' and tests. - This is not used anywhere (yet). Result: Handler state machine is in place.
Motivation: SwiftNIO raised its minimum supported Swift version to 5.4 in the 2.40.0 release. To be able to use the latest updates, we must do the same. Modifications: - Bump swift-tools-version to 5.4 in the various package manifests (including QPS and FuzzTesting) - Add excludes for non-source files (READMEs and protos) - Use 'executableTarget' instead of 'target' for executables - Update the README to reflect supported versions - Remove 5.2 and 5.3 from CI - Remove LinuxMain.swift as test discovery is enabled by default Result: Minimum supported Swift version is 5.4 (cherry picked from commit edcd6b9)
Motivation: Sendable conformance was missing in a handful of places which generates warnings in newer toolchains. Modifications: - Make client calls Sendable - Client call wrappers should require Sendable types (this was implicitly enfored by calls requiring request and response types to be Sendable). This also requires the async sequence of requests to be Sendable for calls where requests are streamed which required a codegen change. - Make the various gRPC async sequence types conditionally Sendable Result: Fewer warnings
Motivation: In grpc#1394 and grpc#1396 we introduced new state machines for the server interceptors and handler. This change updates the async server handler to make use of them. Modifications: - Add the relevant `@inlinable` and `@usableFromInline` annotations to both state machines. - Refactor the async server handler to use both state machines. - Refactor async handler tests to use a 'real' event loop; the previous mix of embedded and async was unsafe. - Re-enable TSAN Result: - Better separation between interceptors and user func - TSAN is happier - Resolves grpc#1362
Motivation: The async call context provided to async server methods provides an API to access request headers and the logger as well as modify the response headers, trailers and user info. This is currently implemented as a class with its mutable state protected by a lock. This works fine but is racy: the response headers can be updated after writing the first message and still be sent before that message (as writing the message requires executing onto the event loop). Modifications: - Turn the context into a `struct` and break it into request and response components. - Request components are immutable (or, more correctly, mutating them has no impact on the underlying RPC). - Response components are mutable via `async` calls which execute onto the underlying event loop. - This introduces allocations as the work is submitted onto the event loop, however setting headers is not a frequent operation and is worth the trade off for a less surprising API. - Trap when headers/trailers are set after they have been sent (this is an assertion failre). Result: - Async server context has a less surprising API - Setting headers/trailers is not racy
Motivation: It's possible to write messages on the client request stream after the RPC has closed. It shouldn't be. Modifications: - Close the request stream on end and on error for client streaming and bidirectional streaming rpcs Result: Callers can't write to a request stream after an RPC has finished.
Motivation: Writes on the request stream should suspend before the RPC is ready. That's not the case right now. Modifications: - Allow the writability of a request stream writer to be specified when initialized and have clients default to not writable. - Plumb through an `onStart` callback which is triggered on the `channelActive` of the HTTP/2 stream which toggles the writability. Result: Attempting to write on a request stream before the underlying http/2 stream is ready will suspend.
Motivation: Cancelling an RPC as a client is done via a cancellation function on the call which `throws` and is also `async`. That it `throws` is odd as an API because by virtue of cancelling we no longer care about the result so it doesn't matter if cancellation was successful or not (i.e. if the RPC was already cancelled). That the cancellation is `async` does not fit with task cancellation handlers which are also not `async`. Modifications: - Make all async call `cancel()` sync and non throwing. - Add tests for the 'wrapped' RPCs (and fix some bugs along the way) Result: Cancelling RPCs from the client is async and more robust.
…rpc#1414) Motivation: Whenever we create continuations we should be careful to add support for co-operative cancellation via a cancellation handler. Modifications: - Add co-operative cancellation to the async write and passthrough source - Tests Result: Better cancellation support
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.
Motivation: Our examples should at least attempt to be up-to-date. The PCAP example currenly uses the NIO based API, we should switch it to the async API. Modifications: - Update the PCAP example to use async/await - Ditch logging for print Result: PCAP example is async/await
Motivation: The Echo example has an async and a non-async version. We should reduce it down to just the async version. Modifications: - Move the async example to the directory of the existing non-async example - Add a missing `@main` to the async example Result: Echo example is async
FranzBusch
approved these changes
Jun 8, 2022
| @@ -3,7 +3,7 @@ on: | |||
| push: | |||
| branches: [main] | |||
| pull_request: | |||
| branches: [main] | |||
| branches: [main, 1.7.1-async-await] | |||
There was a problem hiding this comment.
We can probably drop this again?
There was a problem hiding this comment.
Yup, will do in a follow up. I have a few other fixes that are coming out of the merge but wanted to separate them from the merge to make them more obvious.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
We plan on release async/await soon and have periodically
merged main into the async branch. Now, however, we should
merge the async branch into main and make any final changes
there.
Modifications:
1.7.1-async-awaitintomainResult:
maincontains the async code.