should IdleStateHandler close the connection instead of emitting an event?

[tomerd]

today IdleStateHandler issues an IdleStateEvent on timeouts. in some cases, its better if it would close the connection, for example when using ByteToMessageHandler and/or MessageToByteHandler. see https://github.com/tomerd/swift-nio-examples/pull/1

we should consider replacing the event based behavior or adding a configuration option. i prefer the latter

[normanmaurer]

@tomerd in netty we always send an event and then another handler can act on the event.

[Lukasa]

I am also inclined to say that a different handler should catch this event.

[tomerd]

@Lukasa @normanmaurer this came up in my conversations with @weissi regarding the new ByteToMessageDecoder and MessageToByteEncoder in swift-nio 2 which are a departure from the classic netty handler model. for reference, we had to fix the codec using a separate handler (as you suggest) in https://github.com/tomerd/swift-nio-examples/pull/1

the issue here are: 1/ swift-nio 2 ByteToMessageDecoder and MessageToByteEncoder do not let you handle events, such asIdleStateEvent from IdleStateHandler 2/ its not uncommon to need to handle timeout in framing to handle the "some bytes arrived but they are not a full frame" use case. this can be handled in ByteToMessageDecoder::decodeLast only if the connection is closed on timeout

given the above two, its likely common for framing implementations to need and add this extra "close- connection-on-timeout" handler on top of IdleStateHandler and the discussion i am trying to have here is if we can do better than that

[Lukasa]

For clarity, users will always need to add a handler that handles the idle state events if they want to use that handler.

The generally encouraged NIO design philosophy is to separate concerns. The entities handling close on timeout should be as well separated as possible from the ones handling simple protocol parsing.

The new ByteToMessageDecoder API is focused on specifically enabling correct implementation of a parser. A parser should, in my opinion, not know about timeouts, or indeed anything at all about time. Its only relevant inputs are “more bytes arrived” and “no more bytes will be coming”.

My read of your issue is that you agree; you’re using the timeout for a “no more bytes will be arriving” signal. The key thing to clarify here is that your separate handler for closing will provide that signal. When a channel is closed by either the local or remote peer, that close will trigger decodeLast in the B2MD. This is the signal you’re looking for.

I think this is what you’re looking for, right?

[Lukasa]

I wanted to separately address this note:

given the above two, its likely common for framing implementations to need and add this extra "close- connection-on-timeout" handler on top of IdleStateHandler and the discussion i am trying to have here is if we can do better than that

I think the default NIO view is that more handlers is not by default a bad thing. They have a minor performance cost, but in general they allow better decomposition of responsibilities.

The way we could most naturally do better would be to provide a default implementation of this other handler that does just close on idle timeout. That will at least reduce the need for users to write their own.

[bofeizhu]

@tomerd Some protocols, MQTT for example, requires the client to send ping packets when channel is idle for some time. Closing connection is not a universal behavior for all the network protocols.

[wlisac]

@bofeizhu MQTT is a good example.

I've been working on a SwiftNIO MQTT client and I'm listening for idle write events from an IdleStateHandler to know to send a ping to keep the connection alive.

[tomerd]

The way we could most naturally do better would be to provide a default implementation of this other handler that does just close on idle timeout. That will at least reduce the need for users to write their own.

that sounds good