apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
7.92k stars 643 forks source link

Type-safe Pipeline #1825

Open dnadoba opened 3 years ago

dnadoba commented 3 years ago

Hi everyone,

I have played around with a type-safe Pipeline Builder by using phantom-types. It makes use of the associated types from ChannelInboundHandler and ChannelOutboundHandler to guarantee that handlers have compatible input and output types.

Pipeline Builder is a struct with two generic parameters: InboundIn and OutboundOut:

struct PipelineBuilder<InboundIn, OutboundOut> {
     private var handlers: [ChannelHandler]
}

A Pipeline is started with some start InboundIn and OutboundOut types, e.g.:

let pipeline = PipelineBuilder(inboundIn: ByteBuffer.self, outboundOut: ByteBuffer.self)

After that, we can add channel handlers with the add method:

let pipeline = PipelineBuilder(inboundIn: ByteBuffer.self, outboundOut: ByteBuffer.self)
        .add(FrameDecoderHandler())

pipeline does now have the type PipelineBuilder<Frame, ByteBuffer>. We can also add an encoder:

let pipeline = PipelineBuilder(inboundIn: ByteBuffer.self, outboundOut: ByteBuffer.self)
        .add(FrameDecoderHandler())
        .add(FrameEncoderHandler())

and now pipeline is of type PipelineBuilder<Frame, Frame>.

Trying to add an InboundChannelHandler which does not consumes Frames will fail to compile. The Pipeline can then be added to a real ChannelPipeline by calling channel.pipeline.addHandlers(pipeline).

You can find a prototype implementation in the RSocket repository with a a real world example: https://github.com/rsocket/rsocket-swift/blob/5ec54700b3a00a734e4c31962dc72979339e75df/Sources/RSocketCore/ChannelPipeline.swift#L20-L172

What do you think? Worth adding something like this to SwiftNIO? Found some old draft PR which also tries to make SwiftNIO more type-safe but more focused on the channel handler side and not on the pipeline.

Note: Just played around with it for ~1 hour so no real world usage experience yet.

Lukasa commented 3 years ago

I think this represents an interesting idea.

It's not got the full fidelity we want: ideally we'd support compile-time correctness of pipeline modification as well. However, that's necessarily hard, and this gets us a lot closer. @weissi what do you think of this idea?

weissi commented 3 years ago

I'm totally open to adding such functionality if users think it helps them. The pipeline itself cannot be compile-time type safe because we support mutation but we can offer helpers that make it easier to build a type-safe pipeline. Those helpers would obviously not cover 100% of the use cases but would maybe help for a good chunk of the standard use cases.

I'd think we could add an API that looks something like.

channel.pipeline.makePipelineBuilder()
      .add(HandlerFoo())
      .add(HandlerBar())
      .add(HandlerBuz())
      .build()

or so where the makePipelineBuilder could be similar to what's proposed here and the .build() would then just call into channel.pipeline.addHandlers(...) and actually perform the job.

I think this could be a nice middle ground where many NIO programmers can use the type-safe API without losing any performance (or fidelity) at runtime.

Lukasa commented 3 years ago

When it comes to "Swifty" APIs we should ask ourselves whether a ResultBuilder is an appropriate way to express this API. It's potentially hard for us to use one, but still worth investigating.

weissi commented 3 years ago

When it comes to "Swifty" APIs we should ask ourselves whether a ResultBuilder is an appropriate way to express this API. It's potentially hard for us to use one, but still worth investigating.

We should definitely investigate. I don't think it'll work well for us (5.0 support / mix of types) but it's 100% worth looking into this.

weissi commented 3 years ago

@dnadoba Some other thoughts that I should probably leave here.

The biggest reason I didn't further pursue #1139 is that many many handlers don't actually care what the inbound/outbound data types are because they just let them through. Ie. they don't implement both write and channelRead so the default implementation will just hand that through. That however makes the type-matching hard. In your proposal, this is somewhat less of a concern because you could overload the .add(...) functions so for a ChannelInboundHandler you could only check that the Inbound types match. That would work for more handlers, however there are still problems:

weissi commented 1 year ago

related/dupe of #39