Details

    • Type: Feature Request Feature Request
    • Status: Closed Closed (View Workflow)
    • Priority: Major Major
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: 3.1.0.ALPHA4
    • Component/s: Handler
    • Labels:
      None
    • Similar Issues:
      None

      Description

      Netty core doesn't detect an idle channel at all. There should be a ChannelHandler which triggers some action when a connection becomes idle. For example, a connection which didn't send a certain message for a certain amount of time could be closed automatically. To fulfill this requirement,

      1) A user should be able to define what an 'idle' channel is. (sensible default would be 'no communication for 10 seconds' for example)
      2) A user should be able to define what action should be performed when a channel becomes idle. (sensible default would be disconnection for example)
      3) A user should be able to add multiple instances of this handler so that one can enforce multiple rules.

        Issue Links

          Activity

          Hide
          Christian Migowski
          added a comment -

          I think the idle-issue should be integrated as events like it is done in Mina. Havening it integrated as e.g. channelIdle(...) in SimpleChannelHandler makes implementing protocols with heartbeat messages as easy as
          e.getChannel().write(new HeartbeatMessage());
          therefor holding all protocol-related things together in one class.

          I also can't think of more idle events per Channel than "no incoming data for a while", "no outgoing data for a while" or both (again, like it is done in Mina).

          Show
          Christian Migowski
          added a comment - I think the idle-issue should be integrated as events like it is done in Mina. Havening it integrated as e.g. channelIdle(...) in SimpleChannelHandler makes implementing protocols with heartbeat messages as easy as e.getChannel().write(new HeartbeatMessage()); therefor holding all protocol-related things together in one class. I also can't think of more idle events per Channel than "no incoming data for a while", "no outgoing data for a while" or both (again, like it is done in Mina).
          Hide
          Trustin Lee
          added a comment -

          That's also an option I'm thinking about. However, I'm somewhat concerned about the performance implications of running idleness detection in I/O thread. Idleness detection is performed periodically, but it scans through the whole list of open channels which can be time consuming (i.e. O(N) operation.)

          Therefore, at least I want to make it run in a separate thread with lower priority. Does it make sense to you?

          Show
          Trustin Lee
          added a comment - That's also an option I'm thinking about. However, I'm somewhat concerned about the performance implications of running idleness detection in I/O thread. Idleness detection is performed periodically, but it scans through the whole list of open channels which can be time consuming (i.e. O(N) operation.) Therefore, at least I want to make it run in a separate thread with lower priority. Does it make sense to you?
          Trustin Lee
          made changes -
          Field Original Value New Value
          Fix Version/s 3.1.0.ALPHA3 [ 12312996 ]
          Fix Version/s 3.1.0.ALPHA2 [ 12312995 ]
          Hide
          Christian Migowski
          added a comment -

          If the event is fired close to the time it was configured it doesn't matter how it is done internally (from my API-users point of view).
          The idea to do it in a seperate thread seems reasonable to me (although I didn't know it is time-consuming; but then I am not very familiar with the Java networking internals).

          Show
          Christian Migowski
          added a comment - If the event is fired close to the time it was configured it doesn't matter how it is done internally (from my API-users point of view). The idea to do it in a seperate thread seems reasonable to me (although I didn't know it is time-consuming; but then I am not very familiar with the Java networking internals).
          Hide
          Trustin Lee
          added a comment -

          I'm not very confident that it will be a very time consuming task. However, it is still true that it takes a constant amount of time.

          A separate thread approach adds another overhead of maintaining a map of all open channels in a thread-safe way, which is likely to cause contention.
          I'm thinking of adjusting idleness check interval dynamically from 1 second to 3 seconds depending on the number of connections. WDYT?

          Show
          Trustin Lee
          added a comment - I'm not very confident that it will be a very time consuming task. However, it is still true that it takes a constant amount of time. A separate thread approach adds another overhead of maintaining a map of all open channels in a thread-safe way, which is likely to cause contention. I'm thinking of adjusting idleness check interval dynamically from 1 second to 3 seconds depending on the number of connections. WDYT?
          Hide
          Christian Migowski
          added a comment -

          This sounds like a good idea!
          It should be noted in the documentation that with many channels the "idle events" can be a bit delayed as a compromise to performance.
          That should be ok for almost everyone I guess, since a server with many connections may not answer to his clients in a precise timeframe anyway because it is very busy doing lots of business logic.

          Show
          Christian Migowski
          added a comment - This sounds like a good idea! It should be noted in the documentation that with many channels the "idle events" can be a bit delayed as a compromise to performance. That should be ok for almost everyone I guess, since a server with many connections may not answer to his clients in a precise timeframe anyway because it is very busy doing lots of business logic.
          Trustin Lee
          made changes -
          Fix Version/s 3.1.0.ALPHA4 [ 12313027 ]
          Fix Version/s 3.1.0.ALPHA3 [ 12312996 ]
          Hide
          Nuutti Kotivuori
          added a comment -

          Idleness detection is usually done by a heap (or priority queue) which contains the deadlines on which to fire idle events - and a single timer pending on the first idle event to fire. Updating this heap on each write or read operation (and only for the connections which have the idle timeout specified) should not be a problem. There should be no need to do a scan over the whole list of open channels!

          Also do note that the "idle" detection is part of a more generic set of problems regarding connection keepalive - where the logic could be something along the lines of: 1) send keepalive if there's no traffic in both directions for 20 seconds, 2) wait for keepalive retry for 10 seconds, 3) terminate connection if no reply received.

          Show
          Nuutti Kotivuori
          added a comment - Idleness detection is usually done by a heap (or priority queue) which contains the deadlines on which to fire idle events - and a single timer pending on the first idle event to fire. Updating this heap on each write or read operation (and only for the connections which have the idle timeout specified) should not be a problem. There should be no need to do a scan over the whole list of open channels! Also do note that the "idle" detection is part of a more generic set of problems regarding connection keepalive - where the logic could be something along the lines of: 1) send keepalive if there's no traffic in both directions for 20 seconds, 2) wait for keepalive retry for 10 seconds, 3) terminate connection if no reply received.
          Hide
          Trustin Lee
          added a comment -

          Thanks for your insightful comment, Nuuti. Let me try your suggestion definitely.

          Show
          Trustin Lee
          added a comment - Thanks for your insightful comment, Nuuti. Let me try your suggestion definitely.
          Hide
          Trustin Lee
          added a comment -

          Just finished the implementation of HashedWheelTimer: http://tinyurl.com/7ka4ey
          I used Hashed Timing Wheel which is described here: http://www.cse.wustl.edu/~cdgill/courses/cs6874/TimingWheels.ppt
          What's left is ChannelHandlers that utilizes HashedWheelTimer (i.e. ReadTimeoutHandler and WriteTimeoutHandler.)

          Show
          Trustin Lee
          added a comment - Just finished the implementation of HashedWheelTimer: http://tinyurl.com/7ka4ey I used Hashed Timing Wheel which is described here: http://www.cse.wustl.edu/~cdgill/courses/cs6874/TimingWheels.ppt What's left is ChannelHandlers that utilizes HashedWheelTimer (i.e. ReadTimeoutHandler and WriteTimeoutHandler.)
          Hide
          Trustin Lee
          added a comment -

          ReadTimeoutHandler has been implemented succesfully, but WriteTimeoutHandler was not implemented yet, because Netty doesn't provide an efficient way to get the number of sent bytes for every socket write operation unlike read operations.

          An explicit mechanism to monitor the amount of outgoing data should be provided to implement write timeout efficiently, but it should not make the Channel API bloated unnecessarily.

          Show
          Trustin Lee
          added a comment - ReadTimeoutHandler has been implemented succesfully, but WriteTimeoutHandler was not implemented yet, because Netty doesn't provide an efficient way to get the number of sent bytes for every socket write operation unlike read operations. An explicit mechanism to monitor the amount of outgoing data should be provided to implement write timeout efficiently, but it should not make the Channel API bloated unnecessarily.
          Hide
          Christian Migowski
          added a comment -

          ia the idea of integrating the timeouts into the "standard handler API" (see my first comment) abandoned? I found it very convenient and elegant in Mina to just implement the sessionIdle(...) method without having more code clutter elsewhere.

          Show
          Christian Migowski
          added a comment - ia the idea of integrating the timeouts into the "standard handler API" (see my first comment) abandoned? I found it very convenient and elegant in Mina to just implement the sessionIdle(...) method without having more code clutter elsewhere.
          Hide
          Trustin Lee
          added a comment -

          The current implementation notifies your handler with an ExceptionEvent (ChannelReadTimeoutException or ChannelWriteTimeoutException). The cost of creating an exception is zero because it's pre-instantiated. Would this work for you?

          Show
          Trustin Lee
          added a comment - The current implementation notifies your handler with an ExceptionEvent (ChannelReadTimeoutException or ChannelWriteTimeoutException). The cost of creating an exception is zero because it's pre-instantiated. Would this work for you?
          Hide
          Trustin Lee
          added a comment -

          ... which means the idea of integrating the timeouts into the standard handler API was not abandoned. You can still put the idle connection handler code in your handler implementation. It's not cluttered across different classes. However, your exception handler will have one more if statement:

          public void exceptionCaught(ctx, evt) {
          if (evt.getException() instanceof ReadTimeoutException)

          { // send a heartbeat message or close the connection. }

          else if ( ... instanceof ...)

          { ... }

          }

          ... or ...

          public void exceptionCaught(ctx, evt) {
          try

          { throw evt.getException(); }

          catch (ChannelREadTimeoutException e)

          { ... }

          catch ( ... ) {
          }
          }

          I don't think this change will lead the handler implementation into 'instanceof' hell because your application should already have various exception types which should be handled in exceptionCaught handler method.

          At last but not least, I'd like to hear what you think about the current implementation. Thanks in advance!

          Show
          Trustin Lee
          added a comment - ... which means the idea of integrating the timeouts into the standard handler API was not abandoned. You can still put the idle connection handler code in your handler implementation. It's not cluttered across different classes. However, your exception handler will have one more if statement: public void exceptionCaught(ctx, evt) { if (evt.getException() instanceof ReadTimeoutException) { // send a heartbeat message or close the connection. } else if ( ... instanceof ...) { ... } } ... or ... public void exceptionCaught(ctx, evt) { try { throw evt.getException(); } catch (ChannelREadTimeoutException e) { ... } catch ( ... ) { } } I don't think this change will lead the handler implementation into 'instanceof' hell because your application should already have various exception types which should be handled in exceptionCaught handler method. At last but not least, I'd like to hear what you think about the current implementation. Thanks in advance!
          Hide
          Trustin Lee
          added a comment -

          WriteTimeoutHandler has been implemented rather simply: http://tinyurl.com/am2vel

          I just set an individual timeout for each write request. It just works fine although it's not highly efficient - it creates one timeout and one future listener (and potentially one ArrayList with small initial capacity) for each write request, which might be expensive for some cases. It should be fine in most cases though because a user can disable or enable a certain type of messages by overriding WriteTimeoutHandler.getTimeoutNanos(...).

          If more complicated or customized timeout scheme is required, a user can always implement one's own handler using HashedWheelTimer, channelInterestChanged event, and writeComplete event (see NETTY-118).

          Show
          Trustin Lee
          added a comment - WriteTimeoutHandler has been implemented rather simply: http://tinyurl.com/am2vel I just set an individual timeout for each write request. It just works fine although it's not highly efficient - it creates one timeout and one future listener (and potentially one ArrayList with small initial capacity) for each write request, which might be expensive for some cases. It should be fine in most cases though because a user can disable or enable a certain type of messages by overriding WriteTimeoutHandler.getTimeoutNanos(...). If more complicated or customized timeout scheme is required, a user can always implement one's own handler using HashedWheelTimer, channelInterestChanged event, and writeComplete event (see NETTY-118 ).
          Hide
          Trustin Lee
          added a comment -

          Marking as resolved for a while rather than closing so that people can reopen and leave a comment about one's concern. Here's the latest revision which includes getTimeoutNanos(): http://tinyurl.com/boswgl

          Show
          Trustin Lee
          added a comment - Marking as resolved for a while rather than closing so that people can reopen and leave a comment about one's concern. Here's the latest revision which includes getTimeoutNanos(): http://tinyurl.com/boswgl
          Trustin Lee
          made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Done [ 1 ]
          Trustin Lee
          made changes -
          Link This issue is related to NETTY-118 [ NETTY-118 ]
          Hide
          Christian Migowski
          added a comment -

          I must admit that I am not terribly familiar with Nettty (yet), and there do not seem to be jUnit tests for this now, could you please give a brief example of how to setup the bootstrap (and constructing instances of the *TimeoutHandler classes, this is what I didn't get right now) to be able to both receive read and write timeouts?

          Show
          Christian Migowski
          added a comment - I must admit that I am not terribly familiar with Nettty (yet), and there do not seem to be jUnit tests for this now, could you please give a brief example of how to setup the bootstrap (and constructing instances of the *TimeoutHandler classes, this is what I didn't get right now) to be able to both receive read and write timeouts?
          Hide
          Trustin Lee
          added a comment -

          In your ChannelPipelineFactory, you can add ReadTimeoutHandler and WriteTimeoutHandler. For example:

          public class MyChannelPipelineFactory implements ChannelPipelineFactory {
          // You need to call timer.stop() when shutting down the application.
          private final Timer timer = new HashedWheelTimer(...);

          public ChannelPipeline getPipeline()

          { ChannelPipeline p = Channels.pipeline(); p.addLast("readTimeout", new ReadTimeoutHandler(timer, 10, TimeUnit.SECONDS)); p.addLast("writeTimeout", new WriterTimeoutHandler(timer, 10, TimeUnit.SECONDS)); ... p.addLast("handler", new MyHandler()); }

          }

          The remaining steps are same as ordinary Netty examples. Please pick one that uses ChannelPipelineFactory.

          Show
          Trustin Lee
          added a comment - In your ChannelPipelineFactory, you can add ReadTimeoutHandler and WriteTimeoutHandler. For example: public class MyChannelPipelineFactory implements ChannelPipelineFactory { // You need to call timer.stop() when shutting down the application. private final Timer timer = new HashedWheelTimer(...); public ChannelPipeline getPipeline() { ChannelPipeline p = Channels.pipeline(); p.addLast("readTimeout", new ReadTimeoutHandler(timer, 10, TimeUnit.SECONDS)); p.addLast("writeTimeout", new WriterTimeoutHandler(timer, 10, TimeUnit.SECONDS)); ... p.addLast("handler", new MyHandler()); } } The remaining steps are same as ordinary Netty examples. Please pick one that uses ChannelPipelineFactory.
          Hide
          Christian Migowski
          added a comment -

          Now I've tested it I see that you can really put the idleness detection code in your handler, thanks!
          However, in my opinion the fact that an timeout occured should not be propagated as an exception but as "timeout/idle event", because it is not really an exception but something you expect (after all, you added the handlers). But you are right, the code isn't much cluttered when you handle it as an exception (it would be just more obvious what happens as an explicit event).

          Show
          Christian Migowski
          added a comment - Now I've tested it I see that you can really put the idleness detection code in your handler, thanks! However, in my opinion the fact that an timeout occured should not be propagated as an exception but as "timeout/idle event", because it is not really an exception but something you expect (after all, you added the handlers). But you are right, the code isn't much cluttered when you handle it as an exception (it would be just more obvious what happens as an explicit event).
          Hide
          Christian Migowski
          added a comment -

          We've followed this up extensively on the mailing list, IMHO the idleness detection is complete and fully functional now, i.e. this issue can be closed from my point of view.

          Show
          Christian Migowski
          added a comment - We've followed this up extensively on the mailing list, IMHO the idleness detection is complete and fully functional now, i.e. this issue can be closed from my point of view.
          Hide
          Trustin Lee
          added a comment -

          Oh yes. I gotta close it. Thanks for your insightful yet prompt feed back!

          Show
          Trustin Lee
          added a comment - Oh yes. I gotta close it. Thanks for your insightful yet prompt feed back!
          Trustin Lee
          made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Trustin Lee
              Reporter:
              Trustin Lee
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: