Skip to content
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

Add the router and service protocol #1717

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Nov 22, 2023

Motivation:

The transport layer accepts streams from the client and transforms them into requests which are handled by services. Services, written by users, need a way to tell the server which streams should be handled by which methods.

Modifications:

  • Add the RPC router which routes streams to "handlers"
  • Add the RegistrableRPCService protocol which services can conform to to register routes with a router.

The RegistrableRPCService is typically not implemented by users and instead will be implemented in the generated code. However, it builds on public API offered by the router.

Result:

We have interfaces which allow streams to be routed to a service handler.

Motivation:

The transport layer accepts streams from the client and transforms them
into requests which are handled by services. Services, written by users,
need a way to tell the server which streams should be handled by which
methods.

Modifications:

- Add the RPC router which routes streams to "handlers"
- Add the RegistrableRPCService protocol which services can conform to
  to register routes with a router.

The `RegistrableRPCService` is typically not implemented by users and
instead will be implemented in the generated code. However, it builds on
public API offered by the router.

Result:

We have interfaces which allow streams to be routed to a service
handler.
@glbrntt glbrntt added the v2 A change for v2 label Nov 22, 2023
Sources/GRPCCore/Call/Server/RPCRouter.swift Show resolved Hide resolved
Comment on lines +84 to +97
public var methods: [MethodDescriptor] {
Array(self.handlers.keys)
}

/// Returns the number of methods registered with the router.
public var count: Int {
self.handlers.count
}

/// Returns whether a handler exists for a given method.
///
/// - Parameter descriptor: A descriptor of the method.
/// - Returns: Whether a handler exists for the method.
public func hasHandler(forMethod descriptor: MethodDescriptor) -> Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those things have to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's value in having methods (e.g. logging what methods your server serves). You can obviously just query methods instead of using this function but it's then O(n) rather than O(1) which is a bit sad and the main reason to add this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be solved if methods was a Set instead of an Array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, while the lookup would be O(1) creating the set would still be O(n).

/// - Parameter descriptor: A descriptor of the method to remove a handler for.
/// - Returns: Whether a handler was removed.
@discardableResult
public mutating func removeHandler(forMethod descriptor: MethodDescriptor) -> Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Does this need to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want to offer all methods in a service then this is one way to remove the ones you don't want (the other being to reimplement the service protocol).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I am wondering if we should expose this publicly right away. Do we have concrete use-cases in mind where want to do this or did we get a feature request for this before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concrete use case is the interoperability tests. One test uses the generated client to call a method which isn't implemented on the server. At the moment we achieve this by patching the generated code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think having these methods is very low burden: they're unlikely to change and they still make sense if we change the internals of the router.

Comment on lines +84 to +97
public var methods: [MethodDescriptor] {
Array(self.handlers.keys)
}

/// Returns the number of methods registered with the router.
public var count: Int {
self.handlers.count
}

/// Returns whether a handler exists for a given method.
///
/// - Parameter descriptor: A descriptor of the method.
/// - Returns: Whether a handler exists for the method.
public func hasHandler(forMethod descriptor: MethodDescriptor) -> Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be solved if methods was a Set instead of an Array?

@glbrntt glbrntt enabled auto-merge (squash) November 23, 2023 10:33
@glbrntt glbrntt merged commit c1fbfb4 into grpc:main Nov 23, 2023
13 checks passed
@glbrntt glbrntt deleted the gb-rpc-router branch November 23, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 A change for v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants