-
Notifications
You must be signed in to change notification settings - Fork 652
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
base: main
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
10 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
There was a problem hiding this 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:
- The new public types need to be
NIO
prefixed. - The tests need to be substantially more thorough. Ideally we'd create some quite lengthy pipelines.
- I'd like to see some thinking done about protocol negotiation: how does that look in this pattern?
@swift-server-bot add to allowlist |
I have been playing with result builder based pipelines as well but I took it one step further where I actually changed the The reason why I looked into this was to solve the protocol negotiation problem that we are currently facing in our design of the 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 |
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
andByteBuffer
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.