Add new Server object #1721

glbrntt · 2023-11-24T11:04:18Z

Motivation:

The server is the main entry point for running gRPC services. It combines a set of transports, services, and interceptors.

Modifications:

Add the 'Server' and 'ServerError'

Result:

Can piece together transports, interceptors and services

Motivation: The server is the main entry point for running gRPC services. It combines a set of transports, services, and interceptors. Modifications: - Add the 'Server' and 'ServerError' Result: Can piece together transports, interceptors and services

FranzBusch · 2023-11-24T14:44:25Z

Sources/GRPCCore/Server.swift

+/// you can cancel the task running your server. If your services require additional resources that
+/// need their lifecycles managed you should consider using [Swift Service
+/// Lifecycle](https://github.com/swift-server/swift-service-lifecycle).


I love the callout but I think we should be even more specific here. Not only when you need handling additional resources but also when you want to add support to gracefully shutdown the server. WDYT?

Can you be more specific? We support graceful shutdown here...

Maybe we can give a general recommendation that we recommend running the server in a ServiceGroup since that gives you orchestration of multiple services and support for listening to signals to trigger graceful shutdown.

I'm a bit hesitant to document specifics here. I think the best solution is to have a DocC article which runs through using lifecycle with grpc-swift (which we should link to from these docs). It also means there's a single place which needs to be kept up-to-date.

FranzBusch · 2023-11-24T14:46:40Z

Sources/GRPCCore/Server.swift

+    case stopped
+  }
+
+  private let state: LockedValueBox<State>


Interesting. I didn't expect to have a struct with reference semantics in the end. For ServiceLifecycle we decided to use an actor for the ServiceGroup. Here I would have actually expected a class. Especially since you can do weird things like this

var server = Server() server.transport = [SomeTransport()] async let _ = server.run() server.transports = [OtherTransport()] async let _ = server.run()

IMO here having a class for the Server makes more sense.

Yeah, probably should be a class tbh. I started with a struct and didn't think to back out that change when I added the state management.

FranzBusch · 2023-11-24T14:47:51Z

Sources/GRPCCore/Server.swift

+  public var transports: Transports
+
+  /// The services registered which the server is serving.
+  public var services: Services


I am a bit unsure about making those vars. In the ServiceGroup I had the same decision and went with expecting everything in the inits. The reason for this was that you really expect everything to be setup before run() is called and allowing users to change them after run() is called is a bit weird.

IMO these make the API easier to use, passing everything in init is quite awkward when there are potentially many things to pass in and is awkward w.r.t. API evolution. Sure, you can wrap everything up in a config, but that just adds more indirection for users.

I don't see changing things after run() to be an issue to be honest, trying to make changes after run() has been called just makes no sense at all so it's highly unlikely that someone would fall foul to this. We also document quite clearly that this isn't supported.

Hm, I am not fully convinced yet. Let's imagine the following

var server = Server() server.transports = [NIOH2Transport(port: 1234)] async let _ = try await server.run() server.transports = [NIOH2Transport(port: 5677]) async let _ try await server.run()

There really isn't a reason the second run should fail but it does because of the shared state in the struct. At least personally, I find these semantics confusing. I do get the reason why we want a struct here but then maybe we shouldn't share the state across run invocations?

FranzBusch · 2023-11-24T14:48:17Z

Sources/GRPCCore/Server.swift

+  public var interceptors: Interceptors
+
+  /// The state of the server.
+  private enum State {


Can we move the transports & interceptors into the State so we release them as early as possible?

FranzBusch · 2023-11-24T14:49:26Z

Sources/GRPCCore/Server.swift

+
+    for transport in self.transports.values {
+      do {
+        let listener = try await transport.listen()


Interesting. I did not expect this. I expected that we had to create a child task per transport and each transport has a run method as well? In which task do transports run here?

We do have a child task per transport (see handleRequests). We start them all in the same task so we can ensure they all start successfully.

Just to clarify this. Does that mean that beginListening is expected to bind the port? Because so far we always followed the run pattern where inside the run() method call we bind the ports and then consume the inbound connection/data.

listen() is expected to bind the port. listen() is on the transport which is a lower-level API which users aren't expected to interact with. The run() on the server which we do expect users to interact with does do the bind and consume.

gjcairo · 2023-11-26T17:51:59Z

Tests/GRPCCoreTests/ServerTests.swift

+  }
+
+  func testUnimplementedMethod() async throws {
+    try await self.withInProcessClientConnectedToServer(services: []) { client, _ in


Should we also have a test where we do register the service but we also check we get an unimplemented error?

Suggested change

try await self.withInProcessClientConnectedToServer(services: []) { client, _ in

try await self.withInProcessClientConnectedToServer(services: [BinaryEcho()]) { client, _ in

gjcairo · 2023-11-26T17:53:40Z

Tests/GRPCCoreTests/ServerTests.swift

+    let counter = ManagedAtomic(0)
+
+    try await self.withInProcessClientConnectedToServer(
+      services: [],


Same here, may be worth having a test that checks the interceptors are not applied even when we have a service registered but that doesn't implement a given method.

gjcairo · 2023-11-26T18:02:27Z

Tests/GRPCCoreTests/ServerTests.swift

+
+  func testTestRunServerWithNoTransport() async throws {
+    let server = Server()
+    try await server.run()


Would it be reasonable to throw if we try to run a server without a transport configured?

Good question, I thought about this too but decided against it at the time because it was easier to explain in a sentence what the function does. I think I've changed my mind (again) though: the most likely reason for not configuring a transport is a bug, we should make that more obvious so let's throw an error.

gjcairo · 2023-11-26T18:16:40Z

Sources/GRPCCore/Server.swift

+/// ## Starting and stopping the server
+///
+/// Once you have configured the server call ``run()`` to start it. Calling ``run()`` starts each
+/// of the servers transports. A ``ServerError`` is thrown if any of the transports can't be


Suggested change

/// of the servers transports. A ``ServerError`` is thrown if any of the transports can't be

/// of the server's transports. A ``ServerError`` is thrown if any of the transports can't be

Sources/GRPCCore/Server.swift

gjcairo · 2023-11-26T18:48:07Z

Sources/GRPCCore/Server.swift

+
+  /// Signal to the server that it should stop listening for new requests.
+  ///
+  /// By calling this function you indicate to clients that they mustn't make start new requests


Suggested change

/// By calling this function you indicate to clients that they mustn't make start new requests

/// By calling this function you indicate to clients that they mustn't start new requests

gjcairo

Left a tiny nit about a doc but looking good 👍🏼

Sources/GRPCCore/Server.swift

Co-authored-by: Gustavo Cairo <me@gustavocairo.com>

Add new Server object

af5d7ff

Motivation: The server is the main entry point for running gRPC services. It combines a set of transports, services, and interceptors. Modifications: - Add the 'Server' and 'ServerError' Result: Can piece together transports, interceptors and services

glbrntt requested review from gjcairo and stefanadranca November 24, 2023 11:04

glbrntt added the version/v2 Relates to v2 label Nov 24, 2023

glbrntt added 2 commits November 24, 2023 11:08

Help out 5.7

a42f9c7

Avoid using async let

a3f1fc5

FranzBusch reviewed Nov 24, 2023

View reviewed changes

gjcairo requested changes Nov 26, 2023

View reviewed changes

glbrntt added 3 commits November 27, 2023 11:15

Review feedback

8094acf

remove unused explicit captures

4d4a856

fix racy test

2390b97

gjcairo approved these changes Nov 27, 2023

View reviewed changes

Sources/GRPCCore/Server.swift Outdated Show resolved Hide resolved

glbrntt and others added 3 commits November 27, 2023 12:55

Update Sources/GRPCCore/Server.swift

5234216

Co-authored-by: Gustavo Cairo <me@gustavocairo.com>

Merge branch 'main' into gb-server

978bb46

format

23516c6

glbrntt merged commit 3835e14 into grpc:main Nov 27, 2023

glbrntt deleted the gb-server branch November 27, 2023 14:46

glbrntt mentioned this pull request Nov 27, 2023

Rename server #1724

Merged

	try await self.withInProcessClientConnectedToServer(services: []) { client, _ in
	try await self.withInProcessClientConnectedToServer(services: [BinaryEcho()]) { client, _ in

	/// of the servers transports. A ``ServerError`` is thrown if any of the transports can't be
	/// of the server's transports. A ``ServerError`` is thrown if any of the transports can't be

	/// By calling this function you indicate to clients that they mustn't make start new requests
	/// By calling this function you indicate to clients that they mustn't start new requests

Add new Server object #1721

Add new Server object #1721

Uh oh!

Conversation

glbrntt commented Nov 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjcairo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants