First implementation of gRPC based on SwiftNIO

[MrMage]

This is ready to review (and merge after review) now.

As a weekend project, I looked into actually building a functional serving stack based on SwiftNIO. The serving part is actually working, but before proceeding I'd like to collect some feedback.

1. The use-case is that the user calls GRPCServer.start(), providing a generated provider class (see EchoCallHandlerProvider) that in turn calls through to their business logic (EchoGRPCProvider). We translate HTTP2 structs into their HTTP1 equivalents using HTTP2ToHTTP1ServerCodec (to allow for easier gRPC-via-HTTP1 support) and then decode the gRPC wire protocol via HTTP1ToRawGRPCServerCodec. (De-)Serializing messages is done later; see below.
2. Each time a request comes in, a GRPCChannelHandler is created to handle that request's channel.
3. Once that handler has received the request's headers (in particular, the method name), it looks up the corresponding provider and has the provider return a GRPCCallHandler object.
4. Subsequent messages and stream-close events sent on that channel are then passed to (and handled by) that GRPCCallHandler. (We should probably also forward errors to that object.)
5. The GRPCCallHandler itself then (de-)serializes messages to their actual SwiftProtobuf.Message-derived object types and forwards them to the client's code as appropriate. There are are four concrete implementations of that handler, UnaryCallHandler, ClientStreamingCallHandler, ServerStreamingCallHandler and BidirectionalStreamingCallHandler — one for each kind of gRPC request. (We can't do the deserialization any earlier because we don't know the expected types for the handler before the call's metadata has been sent. In particular, GRPCServerCodec is currently unused.)

FYI, EchoServerTests.swift also contains an example for roughly what the generated server code would look like (EchoCallHandlerProvider), and how the user would need to implement their business logic (EchoGRPCProvider).

So, here are a ton remarks. All feedback would be really appreciated, to any of these points (or others):

• Error handling on the receiving side is essentially non-existent (the sending side is fine, thanks to generous use of promises). Currently, we immediately end the stream with a ServerStatus.processingError when encountering message (de-)serialization errors, and if the user passes an error to the status/result promises, we will also forward those to the user, but other than that, we have no error handling.
• • In particular, I don't know how to forward errors encountered in the NIO network layer on to the library consumer. If some of the NIO people could chime in on this, that would be great.
• Converting a gRPC status to trailers and other channel handling should be separated (First implementation of gRPC based on SwiftNIO #281 (comment))
• The tests in EchoServerTests (which uses a regular SwiftGRPC client to connect to the NIO-based server) all pass, except for testBidirectionalStreamingPingPong and testBidirectionalStreamingLotsOfMessagesPingPong (for those, messages seem to be sent correctly, but the final status doesn't arrive).
• The method signatures that the library's consumer would need to implement (see EchoGRPCProvider) still look a bit funky. E.g. instead of passing the headers argument to all EchoGRPCProvider methods, we could always provide the corresponding CallHandler to all those methods (it is already being passed to server-streaming/bidirectional ones), and store the headers inside the CallHandler.
• We might also want to automatically return an error status when {Server,Bidirectional}StreamHandler.sendMessage() fails.
• In general, it would be nice to get a code review from the SwiftNIO team, to make sure I'm not doing anything wrong here.
• Naming is terrible. I have several classes ending on CallHandler, CallHandlerProvider, Provider, Server etc. that could use better names (for example, there is a UnaryCallHandler that services specific unary gRPC calls that inherits from a UnaryResponseHandler that also partially handles client-streaming calls). Suggestions would be very welcome!
• I'm using too much subclassing right now. In particular, all actual request handlers derive from StatusSendingHandler, and sometimes UnaryResponseHandler as well, and those classes probably violate the single-responsibility principle. In general, the code shared between UnaryCallHandler, ClientStreamingCallHandler, ServerStreamingCallHandler and BidirectionalStreamingCallHandler could probably be cleaned up and made more consistent.
• I'm a bit concerned about having to pass GRPCServerHandler and ChannelHandlerContext arguments to the StatusSendingHandler initializer (and those of its subclasses). Feels like a leaky abstraction to me. (Those objects are at least not memory-leaked, though.)
• There a few assertions that I would want to replace with warnings in production.
• We still depend on SwiftGRPC (and in turn cGRPC), but luckily only in a few places. I think the long-term plan would be to move a small subset of SwiftGRPC that the NIO-based code also depends on into a shared library that would then be depended on by both SwiftGRPC and SwiftGRPCNIO.
• There is currently a crash in NGHTTP2Session.purgeOldStreams when sending ~ 1024 requests to one server (see testUnaryLotsOfRequests). This appears related to the StreamManager.maxCachedStreamIDs setting. Not sure if this is a bug in NIOHTTP2 or whether we are leaking channels here.
• {Server,Bidirectional}StreamHandler.sendMessage() (and maybe .sendStatus) should probably return a future that more actions could be chained to (e.g. to handle backpressure and throttle automatically when trying to send too many responses at once on one channel).
• Not sure if the statusPromise/responsePromise approach in StatusSendingHandler/UnaryResponseHandler is beneficial.

/cc @timburks @bmhatfield

[Lukasa]

There is currently a crash in NGHTTP2Session.purgeOldStreams when sending ~ 1024 requests to one server (see testUnaryLotsOfRequests). This appears related to the StreamManager.maxCachedStreamIDs setting. Not sure if this is a bug in NIOHTTP2 or whether we are leaking channels here.

Relevant code: https://github.com/apple/swift-nio-http2/blob/80963ee509e2d82b68c6e576dde9fb126bc9bcb3/Sources/NIOHTTP2/NGHTTP2Session.swift#L285-L292

Well it's certainly probable that this crash is ours. I don't think you leaking channels could make this crash, the force-unwrap that is sitting there seems like the most likely culprit. If none of the streams there are "active", that would lead to exactly this issue. Should be easily reproduced: will do so when I get back from the dentist.

[MrMage]

Well it's certainly probable that this crash is ours. I don't think you leaking channels could make this crash, the force-unwrap that is sitting there seems like the most likely culprit. If none of the streams there are "active", that would lead to exactly this issue. Should be easily reproduced: will do so when I get back from the dentist.

The crash is indeed in the force-unwrap (sorry, forgot to mention), but I didn't pay closer attention to the underlying causes yet. Take your time! (Also, it should be easy to clone this PR and look at what my code is doing, including whether it is leaking channels. I haven't done that yet due to time constraints and lack of experience with NIO.)

[Lukasa]

Ok, the crash is fixed by apple/swift-nio-http2#11.

[Lukasa]

You can work around this by having this handler be a handler-factory: that is, this handler catches the initial inbound messages and, based on the request's method name, generates an appropriate handler and inserts it in the pipeline directly behind itself.

Does that make sense?

[MrMage]

That would definitely make sense; it's essentially what I had tried to in a much more clunky fashion by piping messages to the GRPCChannelHandler. Is there an example on how that insertion process would be done correctly?

[Lukasa]

Not directly, but there is some similar stuff in the HTTPUpgradeHandler: https://github.com/apple/swift-nio/blob/a7fee8d98ffb1c1fd695a92521c1f6e59a98c78b/Sources/NIOHTTP1/HTTPUpgradeHandler.swift#L212-L242

The general idea is that, in general, pipeline manipulating operations on the channel you're currently running on actually complete synchronously. You shouldn't rely on that fact (that is, you should be prepared to buffer data), but you can always assume any requirement to buffer data will be short-lived.

The only thing you're missing is addinghandlers. For that you probably want add(handler:after:), where after can be self.

[MrMage]

Thanks! I have changed the code to directly add the ServerCodec and CallHandlers now and remove the ChannelHandler.

[MrMage]

@Lukasa @weissi do you have any other feedback on this PR? In particular, is the use of SwiftNIO (with e.g. responsePromise and statusPromise) idiomatic? Or am I doing something potentially dangerous, e.g. by keeping a (weak) reference to ctx in StatusSendingHandler? Or maybe "leaking" resources in some other fashion?

Also, do you have recommendations on passing SwiftNIO errors (e.g. when sending/receiving) on to consumers of this library?

If you have a link to any best practices that could help with this, happy to look into that as well.

[weissi]

@MrMage sorry for the delayed response. I only had a brief look so apologies for my questions: The responsePromise seems to be fulfilled when the client is supposed to send a response. And usually the responsePromise is fulfilled when a StatusSendingHandler received channelRead(which then calls some subclass' processMessage when then in turns fulfils this promise). What I don't really understand is why isn't it firing a new event through the pipeline (ctx.fireChannelRead(...))?

For example this function seems to me like a good candidate for a ChannelInboundHandler:

  func get(request: Echo_EchoRequest, headers: HTTPHeaders, eventLoop: EventLoop) -> EventLoopFuture<Echo_EchoResponse> {
    var response = Echo_EchoResponse()
    response.text = "Swift echo get: " + request.text
    return eventLoop.newSucceededFuture(result: response)
  }

couldn't that be something like

final class EchoGRPCProvider: ChannelInboundHandler {
    [...]
    func channelRead(...) {
        [...]
        switch receivedMessage {
            case .get(Echo_EchoRequest):
                var response = Echo_EchoResponse()
                response.text = "Swift echo get: " + request.text
                ctx.write(...(response)).whenFailure { /* error handling */ }
           [...]
       }
    }
}

so I maybe the pipeline could deliver requests inbound and would be delivered responses outbound.

Does that make some sense?

[MrMage]

@weissi thanks for the feedback!

Sticking to the example of the "get" call, the way a unary gRPC call works is that one input message is passed in and one output result (or an error) is returned. I think that can better be reflected by providing the service implementor with an interface that matches this mental model. If the user had to implement a ChannelHandler themselves, they'd need to add boilerplate for the request lifecycle and state handling (which should be abstracted away by the gRPC framework) and would be able to send more than reply, which would violate the request's semantics.

So, my goal is to:

• only provide the user with the information they need to service the request
• only have them implement the code to service the request, with little boilerplate
• only provide them the methods they need to service the request, without e.g. shooting themselves in the foot by returning more than one response where that would be inappropriate

This is similar to Vapor 3's model, where request handler are also passed in the request data and are asked to return a future of the corresponding response. The framework is then tasked with delivering that response to the client by writing it to the outbound channel.

Also, a more sophisticated example involving actual work on the server side (a database request offloaded to the event loop) could be this:

  func get(request: Echo_EchoRequest, headers: HTTPHeaders, eventLoop: EventLoop) -> EventLoopFuture<Echo_EchoResponse> {
    return database.fetchEchoObjects(withText: request.text).first() // this returns an `EventLoopFuture<EchoObject>`
      .then { echoObject in echoObject.title }
  }

Hope that makes sense — let me know if I'm missing something, or if this approach is problematic.

[weissi]

@MrMage cool, that makes a lot of sense. In that case I think StatusSendingHandler for example does two tasks:

1. doing protocol work (like setting the "grpc-status" trailer)
2. taking the handling of the messages out of the ChannelPipeline

Both are totally totally valid things to do but I would suggest to separate the concerns and use two different channel handlers for that. One that does (1) and one that does (2). In that case if the user is a power-NIO user anyway, they could just not put (2) into the pipeline.

For ease of use, you might obviously provide a method that adds both of them together like we do in NIOHTTP1's `configureHTTPServerPipeline1

[MrMage]

This might be ready to review (and merge after review) now.

[MrMage]

Closing and re-opening to trigger another CI run.

[JonasVautherin]

What about trying to add project-carthage to build-carthage-debug (and probably build-carthage) (here)?

[JonasVautherin]

So I tried to run make project-carthage, and it seems like it's going further (failing because log is too long).

I'll keep you posted, but until now I believe it confirms what I was expecting, so it doesn't sound too bad!

[weissi]

👌 looks great

[rebello95]

Mostly comments about force unwraps, error handling, etc. since we talked offline about the overall implementation with the NIO team. Will an example project and docs be added in a follow-up PR? For the time being, might be worth adding a simple readme explaining that this is a WIP

Great work getting this functioning! 🎉

[rebello95]

Doesn't look like we need this based on how it's used below

[MrMage]

I'm keeping this around to ensure the context (and its promises) do not get deallocated prematurely. Removed the FIXME, though.

[MrMage]

I have addressed all comments now; please at least go through the ones I haven't marked yet resolved and resolve them if you are satisfied with the changes.

I have also added a short section to README.md; sample projects will probably come at some point, but I can't guarantee that I will be the one who will implement them. Also, I think the library should be more production-ready before we add sample projects.

[MrMage]

@rebello95 I hope I have addressed all your comments; please take another look.

[rebello95]

Great start for getting NIO/server support working. Let's merge so we can do further updates in smaller PRs 😄 Might want to squash some of the commits prior to merging

[MrMage]

Great, thanks! I've been planning to squash all commits into one, what do you think?

[rebello95]

Up to you if you want to do one or a couple, no strong opinion. Could make sense to break it into core updates, tests, codegen 🤷‍♂️

[timburks]

LGTM - I think squashing this first big commit would be good.

[MrMage]

Squash-merging this in one commit now. Separating the individual areas of concern would require rebuilding the commit history from scratch.

[weissi]

[MrMage]

Yeah, I saw that in one of your PRs. Sadly that can't be used when the return value of a throwing function is relevant, though.

…

On 27. Nov 2018, at 10:34, Johannes Weiss ***@***.***> wrote: @weissi commented on this pull request. In Tests/SwiftGRPCNIOTests/NIOServerTests.swift: > @@ -145,9 +145,9 @@ extension NIOServerTests { completionHandlerExpectation.fulfill() } - try! call.send(Echo_EchoRequest(text: "foo")) - try! call.send(Echo_EchoRequest(text: "bar")) - try! call.send(Echo_EchoRequest(text: "baz")) + XCTAssertNoThrow(try call.send(Echo_EchoRequest(text: "foo"))) exactly, we've added XCTAssertNoThrow basically everywhere in NIO now for that reason. It's still missing in lots of places which is always a pain when debugging something that happened in CI. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[weissi]

Yeah, I saw that in one of your PRs. Sadly that can't be used when the return value of a throwing function is relevant, though.

yeah, return values are annoying with this. In NIO we have https://github.com/apple/swift-nio/blob/master/Tests/NIOTests/TestUtils.swift#L186

func assertNoThrowWithValue<T>(_ body: @autoclosure () throws -> T, defaultValue: T? = nil, message: String? = nil, file: StaticString = #file, line: UInt = #line) throws -> T {