-
Notifications
You must be signed in to change notification settings - Fork 420
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.
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
Async-await: Base types for client implementation #1243
Async-await: Base types for client implementation #1243
Conversation
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start Si -- I left a bunch of comments in line. Most of my comments on the bidirectional call apply to the others too.
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/AsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
/// - message: The message to send. | ||
/// - compression: Whether compression should be used for this message. Ignored if compression | ||
/// was not enabled for the RPC. | ||
public func sendMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should come up with a consistent spelling for sending; on the server I think we talk about "writing" responses rather than sending.
The same goes for "end" -- for async sequences it seems like "finish" is normal term.
What do you think about send(_ request: Request, compression: Compression = .deferToCallDefault)
and finish()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should come up with a consistent spelling for sending; on the server I think we talk about "writing" responses rather than sending.
The current proposed server implementation (#1249) uses "sending" nomenclature, in so much as the AsyncResponseStreamWriter
(realise the type has "writer" in it) exposes the following to the user handler server implementation:
struct AsyncResponseStreamWriter<ResponsePayload> {
func shouldCompress(_ compression: Compression) -> Bool
func sendResponse(_ response: ResponsePayload, compression: Compression = .deferToCallDefault) async throws
}
The same goes for "end" -- for async sequences it seems like "finish" is normal term.
But IIUC this is not an AsyncSequence
-based API. This is type, AsyncBidirectionalStreamingCall`, represents an ongoing RPC, with which you can either send requests or indicate that you would like to finish the RPC (being careful in this description to use neither spelling).
So the current spellings are all "send" verbs, i.e sendMessage
and sendEnd
. Because it's an RPC, it seems reasonable that concluding an RPC does involve sending something.
However, as I mentioned before, the main motivation behind the spellings in these PRs are that they map closely to the spellings in the existing API. We have since discussed how this is not as much of a concern as I originally thought.
What do you think about
send(_ request: Request, compression: Compression = .deferToCallDefault)
andfinish()
?
The motivation here is to make it feel similar to AsyncSequence
but finish()
is not part of this API. It is part of the Continuation
API, which is often used to create an AsyncSequence
. I guess the argument here is that the RPC client is a source for an AsyncSequence
of requests as viewed by the server? If we want to really embrace this narrative, should we be considering yield
too? E.g.
struct AsyncBidirectionalStreamingCall<Request, Response> {
...
func yield(_ request: Request) async throws
func finish() async throws
}
WDYT? At the end of the day I defer to you as the maintainer here. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But IIUC this is not an
AsyncSequence
-based API. This is type, AsyncBidirectionalStreamingCall`, represents an ongoing RPC, with which you can either send requests or indicate that you would like to finish the RPC (being careful in this description to use neither spelling).So the current spellings are all "send" verbs, i.e
sendMessage
andsendEnd
. Because it's an RPC, it seems reasonable that concluding an RPC does involve sending something.
This is a very helpful comment, it highlights that with the current API we are sending on the RPC as opposed to on a request stream. Would it be clearer for RPCs where the client is streaming to expose a writer-like object on which the caller can send messages? This would also allow us to remove some duplication between the bidi and client-streaming calls. WDYT?
The motivation here is to make it feel similar to
AsyncSequence
butfinish()
is not part of this API. It is part of theContinuation
API, which is often used to create anAsyncSequence
.
True -- that's not a good reason for using finish
!
I guess the argument here is that the RPC client is a source for an
AsyncSequence
of requests as viewed by the server? If we want to really embrace this narrative, should we be consideringyield
too? E.g.struct AsyncBidirectionalStreamingCall<Request, Response> { ... func yield(_ request: Request) async throws func finish() async throws }WDYT? At the end of the day I defer to you as the maintainer here. :)
I don't particularly like yield
-- I don't think it's all that obvious in the context of an RPC what's going on although makes more sense on a request stream which the RPC holds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So after some discussion, we could wrap this functionality in a request writer object that is held by the RPC type. This would make it more symmetric with the server handlers, where the user function is provided with a response writer. With this request writer we can use send(_ request:)
spelling and something like finish()
or complete()
.
However, I think we agreed to keep things as they are for this PR because this change would be very closely coupled with the upcoming internal, pausible writer.
@glbrntt have I recalled and/or interpreted our discussion correctly for this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that sounds right to me.
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
…responses Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
@glbrntt I think this is ready for another pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great modulo a few nits and questions!
Sources/GRPC/AsyncAwaitSupport/GRPCAsyncBidirectionalStreamingCall.swift
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/GRPCAsyncBidirectionalStreamingCall.swift
Outdated
Show resolved
Hide resolved
Sources/GRPC/AsyncAwaitSupport/GRPCAsyncClientStreamingCall.swift
Outdated
Show resolved
Hide resolved
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
Signed-off-by: Si Beaumont <[email protected]>
249b5ab
to
4c99b1f
Compare
/// - message: The message to send. | ||
/// - compression: Whether compression should be used for this message. Ignored if compression | ||
/// was not enabled for the RPC. | ||
public func sendMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did we land with GRPCAsync{Client,Bidirectional}StreamingCall
having a request-stream or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unresolved thread above which I found a reply that I wrote but forgot to send. It's here: #1243 (comment).
I thought we were deferring this until we had the writer that supports backpressure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First async-await PR ✅ -- great stuff Si!
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.
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.