Add proposal for v2 stub design #1652

glbrntt · 2023-09-22T08:20:52Z

No description provided.

FranzBusch

Great work. Left some comments inline!

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

FranzBusch · 2023-09-22T10:57:20Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+
+  /// A request created by the client containing a message producer.
+  public struct Stream<Message: Sendable>: Sendable {
+    public typealias Producer = @Sendable (any Writer<Message>) async throws -> Void


Can we have a concrete type here instead? I could imagine that this concrete type could still hold a specialised closure from the transport's underlying writer and in the case of middleware transformations just hold an existential.

Yes; we should do that. I will update this in the next version of this doc.

FranzBusch · 2023-09-22T10:58:17Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+    // Note: the above syntax does not compilee as `AsyncSequence` does not have a
+    //   primary associated type. However the shape of API is illustrative and
+    //   the exact type is to be determined.


I think it would be better to just write GRPCMessageAsyncSequence then. If you expect to use a concrete type in the end.

Right, I expect it will end up like the writer to be honest as we'll no doubt also need to deal with existentials.

FranzBusch · 2023-09-22T11:01:56Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+// MARK: - Supporting types
+
+/// A sink for values which are produced over time.
+public protocol Writer<Element: Sendable>: Sendable {


We probably want to support ~Copyable types here and all the other protocols/generic types as well and have a conditional Copyable conformance. I don't think the latter is expressible yet but we have to keep it in mind.

FranzBusch · 2023-09-22T11:03:32Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+  /// Metadata associated with the error.
+  public var metadata: Metadata


Interesting. Are we expecting to put the detailed error trailer in here or just all trailers>

All trailers. Initially I didn't have metadata in the error type at all and ended up with a wrapper struct pairing an error with metadata which also conformed to Error. It was quite unpleasant to work with (do I catch RPCError or RPCErrorWithMetadata? Or both?).

Ultimately we expect this to represent the final state of a failed RPC where we'd expect to have metadata as well. That's not always the case because the final error can be synthesised on the client side but using an empty Metadata in that case is simpler than having to deal with two different error types anyway.

I agree. This seems like a nice API, I was just confused by the document because it says associated with the error and I wasn't sure what that meant.

FranzBusch · 2023-09-22T11:05:40Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+
+// Generated conformance to `BindableService`.
+extension EchoServiceStreamingProtocol {
+  public func bind(to router: inout RPCRouter) {


Is bind a term of are here in gRPC? I personally think register(with router:) might sound better.

Not especially, it fits with BindableService though. The naming isn't really important -- users are very unlikely to call this directly. We'd probably have API on the server/router to add a service which would just call through to this.

FranzBusch · 2023-09-22T11:06:25Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+/// implement ``EchoServiceProtocol`` which refines this protocol. However, if
+/// you require more granular control over your RPCs then they may implement
+/// this protocol, or methods from this protocol, instead.
+public protocol EchoServiceStreamingProtocol: BindableService, Sendable {


I find the name BindableService interesting. Especially with Service from ServiceLifecycle. What do you think about naming it GRPCService?

I'm open to bikeshedding the naming. I believe grpc-java uses BindableService but I'm not wedded to that. I would be tempted to drop the "GRPC" and just call it Service: all generated code should be fully qualified anyway so would spell it GRPC.Service so I don't think ambiguity would be an issue.

Agreed we can bikeshed later I think we should not use common types in our libraries. We have made a similar decision in Kafka, Redis etc. where we use XXXClient instead of just Client. Service is particularly dangerous since ServiceLifecycle uses it and it is probably quite common to have both in the same scope.

Since we have RPCError maybe just RPCService?

I don't think Service would be an issue for the reasons given above but I'm also not opposed to RPCService.

FranzBusch · 2023-09-22T11:08:25Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+  // Note: there is no generated 'default' handler, the function body defines
+  // the lifetime of the RPC and the RPC is cancelled once the closure exits.
+  // Therefore escaping the message sequence would result in the sequence
+  // throwing an error. It must be consumed from within the handler.


In the future, we should see if can adopt ~Escapable here and let the compiler make sure this is enforced.

Is there a timeline for this?

This was part of the BufferView proposal.

For reference: https://github.com/atrick/swift-evolution/blob/bufferview-roadmap/visions/language-support-for-BufferView.md

FranzBusch · 2023-09-22T11:10:52Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+  let request = ClientRequest.Stream<Echo.Response>(metadata: ["foo": "bar"]) { writer in
+    for text in ["foo", "bar", "baz"] {
+      try await writer.write(Echo.Request(text: text))
+    }
+  }
+
+  // Execute the RPC:
+  try await echo.collect(request: request) { response in
+    // (Same as for the server streaming "Expand")
+  }


I find this spelling very interesting and a bit surprising. The problem that I see is that there are now two separate scopes - one for the request writer and one for the response. If you want to implement business logic where you write a certain request based on a response you received this becomes very hard and needs a layer of indirection.

From previous experience, I found it helpful to have the bidirectional streaming closure provide both the responseStream and the requestWriter.

Just some pseudo code for what I mean

try await someClient.bidistream(metadata: ...) { requestWriter, responseStream in try await requestWriter.write(...) let firstResponse = try await responseStream.first if firstResponse.someProperty { // different message written } else { // different message written } }

I see this a good thing: if your input stream depends on the output stream then you should make the dependency explicit via an additional AsyncStream or similar. Splitting them up makes it harder to accidentally deadlock.

It does make some use cases harder, but they are still possible.

IMO, this is really important. I have myself written a bunch of tests where driving both the requests and responses in tandem is important. This is mostly the case when you implement something like a distributed state machine over a bidirectional streaming RPC.

Having to go through an intermediate stream can work but has two downsides:

It is currently not implementable since we don't have a backpressure AsyncSequence that we could use here.

It becomes less performant since we now have another layer of indirection.

Beyond the issues mentioned above, the request and response being tied together has an impact on the interceptor API: it effectively takes the same shape as the function which isn't at all ergonomic to use. The knock on from this is that the client API diverges even further from the server API.

I'm confident that this is the correct spelling for the base API: it separates the input from the output and makes it hard for users to misuse the API. I

It is currently not implementable since we don't have a backpressure AsyncSequence that we could use here.

We also don't have this API right now! There are backpressure AsyncSequences which do work: AsyncChannel (sure, a bit slow as the buffer size is 1, but it works and is probably suitable for most uses cases) and NIOs producer. There are others in the works too.

It becomes less performant since we now have another layer of indirection.

True, but if your two streams depend on each other then the RTT will likely dominate anyway.

I have myself written a bunch of tests where driving both the requests and responses in tandem is important.

If requested enough we can add a utility or sugar for this. I don't think it should be the base API though.

Jerry-Carter · 2023-09-25T23:56:32Z

I have reviewed the proposal and motivation document.

First, let me strongly endorse the motivation. I concur that "the ecosystem is trending towards using common types and patterns such as Swift HTTP Types 7 and is adopting Swift concurrency in place of EventLoopFuture based APIs" and, as a user, would love to see gRPC-Swift stay in the vanguard. The support for async in 1.x was great to see and 2.x should be even better.

Second, I have reviewed the API. There are no real surprises. GRPC is conceptually fairly simple and the propose API for 2.x is appropriately straightforward and clear.

Third, in any rewrite, I would like to see channel level interceptors added.

Fourth, thought should be given to enhancing gRPC-Swift 2.x by attempting to automatically recover from network issues. I have found gRPC-Swift to work very well between devices with stable connections, but for IOT or mobile clients, the recovery code that currently needs to be written and tested in the client would seem better moved into the library. See 1409 and similar open issues.

Finally, the Using ~Copyable writers section is missing a word: "Initially it may either send metadata or it may return a status as its first and only value."

Thanks, George, Franz, et. al, for doing a great job with 1.x. I look forward to migrating to 2.x ASAP.

glbrntt · 2023-09-26T07:10:04Z

I have reviewed the proposal and motivation document.

Thanks for taking the time to review this, it's great to get feedback from the community 🙏

First, let me strongly endorse the motivation. I concur that "the ecosystem is trending towards using common types and patterns such as Swift HTTP Types 7 and is adopting Swift concurrency in place of EventLoopFuture based APIs" and, as a user, would love to see gRPC-Swift stay in the vanguard. The support for async in 1.x was great to see and 2.x should be even better.

Agreed. I think this is a really exciting time for the Swift and Swift on Server.

Second, I have reviewed the API. There are no real surprises. GRPC is conceptually fairly simple and the propose API for 2.x is appropriately straightforward and clear.

👍

Third, in any rewrite, I would like to see channel level interceptors added.

Don't worry – this is very high up our list. The v1 interceptors are a bugbear of mine which I look forward to rectifying.

Fourth, thought should be given to enhancing gRPC-Swift 2.x by attempting to automatically recover from network issues. I have found gRPC-Swift to work very well between devices with stable connections, but for IOT or mobile clients, the recovery code that currently needs to be written and tested in the client would seem better moved into the library. See 1409 and similar open issues.

Agreed; this is a real weakness of v1. Supporting gRFC A6 out-of-the-box should help somewhat here. However the transport layer will also need to become more resilient in the face of changing network conditions.

Finally, the Using ~Copyable writers section is missing a word: "Initially it may either send metadata or it may return a status as its first and only value."

Thanks. I've fixed that up.

Thanks, George, Franz, et. al, for doing a great job with 1.x. I look forward to migrating to 2.x ASAP.

🙏

vykut · 2023-11-03T10:32:18Z

Sources/GRPC/Docs.docc/Proposals/0001-stub-api.md

+  let request = ClientRequest.Single(message: Echo.Request(text: "foo"))
+
+  // Execute the RPC (most verbose API):
+  await echo.get(request: request) { response in


Hi,

Why is the API modeled in such a way that the consumer would need to pass a closure in order to use the response?
Wouldn't it be more straightforward to just do

let response = echo.get(request: request) // response: ClientResponse.Single<EchoResponse> switch response.result { ... }

IMO, the proposed 'completion handler' pattern is exactly what async/ await ought to get rid of, and to make code more readable. 🤷‍♂️

This is different to a "traditional" completion handler: here the echo.get won't return until the handler has run which means we know that after echo.get returns, all resources associated with the call have been freed.

For unary RPCs this isn't actually an issue because once there's a single response the RPC is done, you can just return the response from the closure. We may actually default the implementation of the closure to do this in generated code.

For RPCs which stream requests and responses we can't do this without resorting to unstructured concurrency which we absolutely want to avoid: cancellation is not propagated to unstructured tasks which makes the underlying code and code which uses these APIs difficult to reason about.

Even for the stream requests, the body closure will be executed only once.
In my mind, the same syntax should work for stream requests as well, i.e

let response = try await echo.expand(request: request) for try await message in response.messages { ... }

The the important part here is this:

For RPCs which stream requests and responses we can't do this without resorting to unstructured concurrency which we absolutely want to avoid: cancellation is not propagated to unstructured tasks which makes the underlying code and code which uses these APIs difficult to reason about.

The echo.expand call is hiding a TaskGroup which runs a number of tasks required to actually execute the RPC. The response handler is running as one of those tasks.

I see. Now that makes sense.
Thanks for your explanation!

glbrntt added the semver/none No version change required. label Sep 22, 2023

Add proposal for v2 stub design

721773b

glbrntt force-pushed the gb-stub-proposal branch from 4f455fd to 721773b Compare September 22, 2023 10:54

FranzBusch reviewed Sep 22, 2023

View reviewed changes

glbrntt added 7 commits September 25, 2023 13:55

Add AsyncSequence variant for writer

b09e269

Add a type-erased async sequence type

eca9bd9

Rename BindableService to RPCService

8dc267e

Add a note about the non-escapable handler

b5b58f6

Add a concrete writer type

dfe8742

Add note about client returning responses

2279fea

Add notes about what changed

0994ec2

Add missing word

193cd02

glbrntt marked this pull request as ready for review October 2, 2023 10:25

FranzBusch approved these changes Oct 2, 2023

View reviewed changes

glbrntt merged commit 5ea1768 into grpc:main Oct 2, 2023
13 checks passed

glbrntt deleted the gb-stub-proposal branch October 2, 2023 11:33

glbrntt added the v2 A change for v2 label Nov 1, 2023

vykut reviewed Nov 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proposal for v2 stub design #1652

Add proposal for v2 stub design #1652

glbrntt commented Sep 22, 2023

FranzBusch left a comment

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

FranzBusch Sep 22, 2023

glbrntt Sep 22, 2023

Jerry-Carter commented Sep 25, 2023

glbrntt commented Sep 26, 2023

vykut Nov 3, 2023 •

edited

Loading

glbrntt Nov 3, 2023

vykut Nov 3, 2023

glbrntt Nov 3, 2023

vykut Nov 4, 2023

		/// Metadata associated with the error.
		public var metadata: Metadata

Add proposal for v2 stub design #1652

Add proposal for v2 stub design #1652

Conversation

glbrntt commented Sep 22, 2023

FranzBusch left a 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

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

Jerry-Carter commented Sep 25, 2023

glbrntt commented Sep 26, 2023

vykut Nov 3, 2023 • 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

vykut Nov 3, 2023 •

edited

Loading