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 a pipeline builder that checks the validity of the order of handlers #2335

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Joannis
Copy link

@Joannis Joannis commented Dec 17, 2022

Add a pipeline builder that checks the order of handlers

Motivation:

While NIO's pipelines provide an exceptionally good framework for networking, there are two main outstanding imperfections in NIO's pipelines that are yet to be improved upon.

First of all, channels depend on NIOAny for unwrapping types, the pipeline handlers types are therefore not checked at compile time, leading to headaches - in particular when working with more complex Channel setups.

In addition to the above, channels require a fair amount of 'extra' calls to glue handlers together. After a handler received a read or write, it applies any logic, and writes their 'Out' type into the next handler in line. To call the next handler, handlers need to invoke a read/write on a 'Context' type which has knowledge of the pipeline. This context then calls the next handler with the same payload. This is suboptimal for the performance of NIO based networking.

While this PR doesn't fix either of those, it _does_reduce the headaches originating from a lack of type-checking. This PR would pave the way for the above two changes to be made in follow-up PRs. Not only does this improve the performance of SwiftNIO. It also improves the maintainability of SwiftNIO based applications significantly, while lowering the entry-barrier for newer network application developers.

Another future direction I can see this leading towards, is the addition of groups of handlers (as Johannes called them 'Fuses'). A 'fuse' could represent an HTTP1 or HTTP2 pipeline. From that change onwards, an HTTPS server would simply add the TLS + SNI handlers on a pipeline. Following the result of SNI, either the HTTP1 or the HTTP2 fuse would be appended to the pipeline, from then on handling the decrypted traffic.

Modifications:

This PR introduces a new ChannelPipelineBuilder, a result builder that uses buildPartialBlock's reducer pattern to check Inbound and Outbound types in both directions. This resultBuilder contains a special case allowing IOData and ByteBuffer to be used interchangeably.

Result:

After these changes, the result builder can be leverages to create a pipeline that is type checked. Immediately following the merge of this change, this will increase maintainability of NIO's pipelines.

This could in future versions lead to the elimination of the extra 'glue' calls within NIO itself. In addition to the above, it can be leveraged to provide APIs on Channels that are aware of the type(s) that can be written to a Channel, based on the type information available from this resultBuilder.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

10 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@Joannis Joannis changed the title Add a pipeline builder that checks the order of handlers Add a pipeline builder that checks the validity of the order of handlers Dec 18, 2022
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I think the general shape of this is really interesting! I'd like to get @FranzBusch to take a quick look at this to confirm that it is potentially useful for some of the long-term pipeline freezing goals we have.

Additionally, we need a few tweaks:

  1. The new public types need to be NIO prefixed.
  2. The tests need to be substantially more thorough. Ideally we'd create some quite lengthy pipelines.
  3. I'd like to see some thinking done about protocol negotiation: how does that look in this pattern?

@Lukasa
Copy link
Contributor

Lukasa commented Mar 2, 2023

@swift-server-bot add to allowlist

@FranzBusch
Copy link
Member

FranzBusch commented Mar 2, 2023

I have been playing with result builder based pipelines as well but I took it one step further where I actually changed the Channel protocol to also require to be fully typed and made the whole pipeline immutable. This worked out quite nicely and allowed to have fully typed pipelines that also supported protocol negotiation etc; however, this was all obviously API breaking and is something we can only consider doing for the next major.

The reason why I looked into this was to solve the protocol negotiation problem that we are currently facing in our design of the NIOAsyncChannel. We need to add the appropriate handlers of the NIOAsyncChannel to the pipeline once all protocol negotiation is done but we still have to do this synchronously otherwise we might lose data.

This PR is adding convenience on top of the current pipeline building to make sure we are not assembling a pipeline with mismatched types but it can still easily be broken with the current mutability. This still adds value but I would like to explore the design channels of NIOAsyncChannel + protocol negotiation with our current APIs first before we introduce these builders to make sure they fit together. So let's make some progress on the NIOAsyncChannel stuff and then we can circle back to this PR and see how it slots in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants