Spontaneous inbound events considered harmful

[guoye-zhang]

One of the most common issues when implementing a network protocol is data truncation. This is frequent encountered on the client side when a fully received response blocked by temporary back pressure gets terminated part way due to a close event or an error. The ultimate cause is that inbound events do not respect back pressure and can be processed out-of-order with reads.

A robust protocol implementation needs to intercept all inbound events and buffer them, finish reading remaining data, then decide whether to forward the inbound events up or drop them. It is hard to exhaustively test all possible event ordering, so this code is often fragile.

I'm creating the issue to start a discussion on how we can help implementors avoid this common pitfall.

[guoye-zhang]

One idea is to generalize read to work with events as well. Callbacks like errorCaught won't be invoked unless there is an active read, and they always get delivered in a separate read loop.

[Lukasa]

Conceptually that proposal is workable, but it represents a real stability risk: I don't think we could ship it without a semver major and an API change that signals the different behaviour. I suspect (though haven't proven) that there are existing implementations relying on the current behaviour.

Additionally, to implement this without changes in the handlers themselves requires the pipeline to essentially take control of event delivery, establishing a read state between each handler. That's not necessarily a bad idea, but it acts as essentially a form of re-entrancy breaking and we'll need to figure out what the performance impact of that change is.

As a final note, not all inbound events are data events: that is, it's potentially ok to re-order some of them in some contexts. For example, many of the HTTP/2 state machine update events don't want to be delayed: the user needs to be aware of them immediately, even if they haven't yet seen the frame that triggered the state change, because the HTTP/2 handler will be enforcing that rule.

My proposal for addressing this might be to consider it part of the "how do we break re-entrancy for most users" conversation. I think the version of that that makes the most sense to me is to introduce a SimpleChannelHandler protocol that removes the AnyObject restriction on ChannelHandler. This would then be wrapped in a ChannelHandler that is responsible for getting everything to fit together. For example, if we considered just a few events:

public protocol SimpleChannelInboundHandler {
    associatedtype InboundIn
    associatedtype InboundOut = Never

    mutating func channelRead(context: SimpleHandlerContext, data: NIOAny) -> [SimpleHandlerEvent]
}

public final class HandlerWrapper<Wrapped: ChannelHandler> {
}

extension HandlerWrapper: ChannelInboundHandler where Wrapped: SimpleChannelInboundHandler {
}

In this pattern, because channelRead (and the other handler witnesses) are mutating, they are also definitionally non-reentrant (per the law of exclusivity). That non-reentrancy means that event ordering has to be handled by the HandlerWrapper, which would therefore become able to enforce the ordering constraints suggested above.

I'd also like to hear inputs from @glbrntt, @weissi, @normanmaurer, @FranzBusch, @Joannis, @gwynne, and anyone else in the Swift server ecosystem who has thoughts on this.

[Joannis]

I'd be all for value types as channel handlers, and it's something I've wanted for ages!

I don't think I rely on inbound events much, mainly notable exception being SSH. Ordering is definitely something I'm not confident I handle correctly, which is a problem.

These days I handle these signals outside of Channel Handlers, modeled as an Async Sequence or something like that. So it doesn't impact me much.

Definitely do see the issue with this though.

[gwynne]

cc @0xTim as well

[FranzBusch]

Overall I'm sympathetic to the problem. Not only from an attack angle but also from a correctness angle. I have implemented and reviewed many channel handlers over the past years and a common problem was half closure events being delivers through the "event-side-channel" which often caused ordering problems when buffering channel handlers weren't correctly buffering them w.r.t. reads and writes. I do agree with @Lukasa point though that doing this in a non-breaking way is the tricky part. I think a non-reentrant handler abstraction could allow us to solve this problem and respell the interface as @Lukasa proposed. However, I'm not yet convinced that it should only be a subset of events (e.g. only data related events) or if it needs to be all inbound and outbound events.

FWIW, our NIOAsyncChannel bridge has solved the data events problem at least. It bridges the half closure and close events into asynchronous primitives. With the only remaining problem that there is no "gentle close" that waits for all outstanding writes to be flushed and the closes.

[guoye-zhang]

I would argue that all events are ultimately data related one way or another, since data is the only thing exchanged over the network.

Thinking about this from another perspective, how many concurrent tasks (to borrow the Swift concurrency terminology) do people need to reason about in a ChannelHandler? Reading and writing have to be concurrent, so that's 2. Every event that can happen out of order w.r.t. reading or writing would be another concurrent task, adding to the complexity of state management exponentially.