Add new GRPCClient object #1727
Conversation
Tests/GRPCCoreTests/Call/Client/ClientRPCExecutionConfigurationCollectionTests.swift
Outdated
Show resolved
Hide resolved
| /// RPCs are intercepted in the order that interceptors are added. That is, a request sent from the client to | ||
| /// the server will first be intercepted by the first added interceptor followed by the second, and so on. | ||
| /// For responses from the server, they'll be applied in the opposite order. | ||
| public var interceptors: Interceptors { |
There was a problem hiding this comment.
What happens if I change interceptors while a request is currently making its way through the interceptor pipeline?
There was a problem hiding this comment.
You can't modify interceptors once the Client is running (and you cannot execute RPCs unless the Client is running either) so this cannot happen.
There was a problem hiding this comment.
Nothing. There's a state check and we grab the array of interceptors before executing the request.
There was a problem hiding this comment.
I see but I also find it a bit weird. Like you are calling interceptors = [] after calling run and that call just gets dropped and nothing happens. Why don't we just make them a part of the init and not allow modifying them afterwards?
There was a problem hiding this comment.
Yeah, I agree, it is a bit weird.
Why don't we just make them a part of the init and not allow modifying them afterwards?
We can do that.
| } | ||
|
|
||
| /// Underlying storage for the client. | ||
| private struct Storage { |
There was a problem hiding this comment.
This is the opposite of what we normally do. We put the storage into the associated values of the State enum. The advantage of that is that once we transition to stopped we free all of the retained storage.
There was a problem hiding this comment.
Not always. We tend to do that when the state management and dependencies are complicated. That's not the case here.
There was a problem hiding this comment.
Sure but I still think there is benefits to that approach like freeing up the resources as soon as possible. Right now we are going to retain the interceptors for longer than we need.
| /// The transport will be closed: this means that it will be given enough time to wait for in-flight RPCs to | ||
| /// finish executing, but no new RPCs will be accepted. | ||
| /// You can cancel the task executing ``run()`` if you want to immediately stop all work. | ||
| public func close() { |
There was a problem hiding this comment.
IMO close is the wrong name here and we should follow the lifecycle naming of triggerGracefulShutdown
There was a problem hiding this comment.
Why do you think it's the wrong name?
There was a problem hiding this comment.
Hm if we follow that convention, shouldn't we also rename stopListening() in GPRCServer to triggerGracefulShutdown too?
cc @glbrntt
There was a problem hiding this comment.
Why do you think it's the wrong name?
At least coming from a NIO world or an FD world close means "close right now". However, from the documentation here it sounds way more like a graceful shutdown than an immediate close. Furthermore, we are kinda trying to establish the pattern of gracefulShutdown with ServiceLifecycle so I think aligning on a common name is beneficial for the ecosystem.
There was a problem hiding this comment.
At least coming from a NIO world or an FD world close means "close right now".
I don't think either of these are correct. In NIO close() just sends a signal down the channel pipeline, a channel handlers can do whatever they want with that signal, including letting in flight requests continue. Closing an FD won't close it right away: the filesystem may need to actually write out data to the underlying storage. The point is that in neither case is close required to take no further action before freeing the underlying resource.
Furthermore, we are kinda trying to establish the pattern of gracefulShutdown with ServiceLifecycle so I think aligning on a common name is beneficial for the ecosystem.
I don't buy into this reasoning at all. Names should be chosen because they are the most appropriate name for the thing they are naming. The graceful shutdown pattern is established for service lifecycle because that's the method name required by its protocol. It is necessarily generic because it can't know what an individual service needs to do to shutdown gracefully. We don't depend on lifecycle here so we don't need to and shouldn't use the generic name.
| @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *) | ||
| extension GRPCClient { | ||
| /// A collection of interceptors providing cross-cutting functionality to each accepted RPC. | ||
| public struct Interceptors: Sendable { |
There was a problem hiding this comment.
Do we ever want to actually make this fully generic? So you can have a GRPCCLient with a generic interceptor pipeline which hopefully gets specialised. @glbrntt
There was a problem hiding this comment.
I don't think the generics carry their weight at the interceptor level tbh. Having a client generic over a transport and a bunch of interceptors hurts ergonomics quite badly.
| @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *) | ||
| extension GRPCClient { | ||
| /// The execution policy for an RPC. | ||
| public enum ExecutionPolicy: Hashable, Sendable { |
There was a problem hiding this comment.
Can we use a struct for this instead?
There was a problem hiding this comment.
We discussed this one previously (it's just been renamed from elsewhere)
gjcairo
force-pushed
the
client-object
branch
from
e7e53d0 to
46443c2
Compare
gjcairo
force-pushed
the
client-object
branch
from
07bff44 to
33f3c70
Compare
|
Superseded by #1729 |
3 participants
Motivation
Clients are the highest level API to make requests to a gRPC server.
Modifications
Add a
GRPCClientResult
We now have a high level client that can be used by the generated stubs to start RPCs to gRPC servers.