Proposal: Async-await client and server APIs #1231
Conversation
Signed-off-by: Si Beaumont <[email protected]>
|
|
|
Thanks you, this looks really great to me! |
| requests: GRPCAsyncStream<Echo_EchoRequest>, | ||
| responseStreamWriter: AsyncResponseStreamWriter<Echo_EchoResponse>, | ||
| context: AsyncServerCallContext | ||
| ) async throws |
There was a problem hiding this comment.
This boils down to opinions™ but worth considering that this still allows the error of forgetting to complete the response stream etc. I noticed this is handled actually, so that's very good 👍
We went through the same design dance way back when with akka-grpc and went with accepting and emitting a stream (Source in Akka's wording); https://doc.akka.io/docs/akka-grpc/current/server/walkthrough.html#implementing-the-service
~It may be that Swift is still too rigid to allow that... we may first need the ability to say some AsyncSequence where Element == Echo_EchoResponse to be honest... 🤔 I believe this feature is coming "soon". ~
The more I looked at this and remembered our semantics for the async for loop will work quite nicely...
There was a problem hiding this comment.
I agree that we would like some AsyncSequence where Element == .... George and I were mulling over the same as this would allow us to use opaque return type without having to provide the GRPCAsyncStream wrapper in this proposal.
There was a problem hiding this comment.
I added a note about the desire for an opaque AsyncSequence return type in da20f7d.
| count += 1 | ||
| try await responseStreamWriter.sendResponse(response) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hm now that I think about it (after the comment above) I guess the "stream writer" is easier to implement for folks since it fits loops so nicely and the entity is re-entrant anyway... Seems the writer approach could be good after all 👍
But you may want to figure out how you want to implement the backpressure here -- would you want to suspend here until more demand is abailable to push more responses? Internally I guess this would be implemented the ill named but final AsyncStream API, so you'll have all the tools to notice backpressure on that boundary, so that's good news 👍
There was a problem hiding this comment.
Stream writer probably handles backpressure by refusing to return from the write call when the window is full. This doesn't prevent multithreaded writing to the stream writer so we need to tolerate that, but this still works pretty well.
Also, the stream writer is a base abstraction: we can absolutely use it to implement an API that accepts an AsyncSequence.
There was a problem hiding this comment.
Stream writer probably handles backpressure by refusing to return from the write call when the window is full.
Ok good 👍
Also, the stream writer is a base abstraction: we can absolutely use it to implement an API that accepts an AsyncSequence.
Yeah, good -- I saw that bit later on in the proposal, happy to see it 👍
There was a problem hiding this comment.
I could add a sentence or two about backpressure to this doc. As you noted, there is no attention given to it. This was a deliberate omission from the prototype because the underlying NIO gRPC library doesn't handle backpressure today. But, as discussed in this thread, we should be able to accommodate backpressure with this API.
There was a problem hiding this comment.
I added a note about (the lack of) backpressure support in 71d3728.
| > different type of call and this context is generic over the response type. | ||
|
|
||
| The same non-generic `AsyncServerCallContext` is passed to the user function | ||
| regardless of the type of RPC. |
| > is nothing that enforces this. | ||
|
|
||
| The user need simply return from the function or throw an error. The closing of | ||
| the call is handled by the library. |
| or a value of a type that conforms to `GRPCStatusTransformable` then the library | ||
| will take care of setting the RPC status appropriately. If the user throws | ||
| anything else then the library will still take care of setting the status | ||
| appropriately, but in this case it will use `internalError` for the RPC status. |
There was a problem hiding this comment.
nitpick, perhaps GRPCStatusError if it is intended to be thrown...? 🤔
There was a problem hiding this comment.
That's a fair callout. GRPCStatus already exists in the API and already conforms to Error so I was working with that. It isn't only used on the failure path which is why I guess it does not have Error in the name.
I suppose we could wrap the GRPCStatus in a struct with a more explicit name. @glbrntt do you have any thoughts on this?
There was a problem hiding this comment.
I agree GRPCStatusError is a better name (and shouldn't allow an 'ok' status) but personally I don't think we buy ourselves much by wrapping a GRPCStatus.
The existing name would still be present in various places throughout the API (we could deprecate it but it's quite pervasive so I'd rather not) and having two very similar types would make the overall picture less clear, I think.
We should revisit when we do a major breaking change though.
| // Wait for the call to terminate, but using `await` rather than a future. | ||
| let status = await update.status | ||
| print("update completed with status: \(status.code)") | ||
| ``` |
There was a problem hiding this comment.
This all looks good 👍
Not in love with sendEnd() I guess (one can miss sending it), but it matches the general style of the stream writer so I guess it's fine 👍
There was a problem hiding this comment.
Yep, this is why we provide the "simpler" API variants below as you already noticed in #1231 (comment).
| In addition to avoiding the pitfall of the expressive counterparts, the client- | ||
| and bidirectional-streaming calls provide the ability to pass an `AsyncSequence` | ||
| of requests. In this way, the underlying library takes care of ensuring that no | ||
| part of the RPC goes unterminated (both the request and response streams). It |
There was a problem hiding this comment.
ok good, this addresses my annoyance with sendEnd mentioned above, good 👍
| requests: RequestStream, | ||
| callOptions: CallOptions? = nil | ||
| ) -> GRPCAsyncStream<Echo_EchoResponse> | ||
| where RequestStream: AsyncSequence, RequestStream.Element == Echo_EchoRequest { ... } |
There was a problem hiding this comment.
Signature looks good (that's the "akka grpc way" I linked/mentioned above, very happy to see it be offered as well here) 👍
There was a problem hiding this comment.
This looks very good, thanks for the writeup! I think it'll fit the concurrency story very well 👍
Tracing should also be able to integrate here; I'm not sure about the "return response stream"-one yet but we'll figure that out (it's a general streaming + tracing question, not specific to grpc)
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
This PR implements some of the types required by the proposal for async/await support, added in #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.
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.
This commit implements some of the types required by the proposal for async/await support, added in #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 #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 #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.
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.
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.
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.
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.
This PR implements some of the types required by the proposal for async/await support, added in #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.
This commit implements some of the types required by the proposal for async/await support, added in #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 #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 #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.
This PR implements some of the types required by the proposal for async/await support, added in #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.
This commit implements some of the types required by the proposal for async/await support, added in #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 #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 #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.
I have spent the last couple of weeks with a few folks at Apple thinking about how we might offer async/await APIs for this project. Specifically how users might be able to implement and call gRPC services using APIs that abstract away NIO and expose functionality in terms of the new native Swift concurrency features.
I have written up our thinking so far in the form of a proposal which is contained in this PR with the hope of gathering feedback from a wider audience. Anyone who has feedback should feel welcome to comment directly on the contents of this PR.
I also have a branch that implements the proposal for those who wish to take a closer look at how it looks and feels: main...simonjbeaumont:async-await-poc-squashed.
Thanks to @glbrntt for the help so far!
/cc @glbrntt @Lukasa @fabianfett @PeterAdams-A