Replace IdleTimeout with CyclicTimeout from #1918

[gregw]

The CyclicTimeout class from #1918 is much simpler code yet substantially the same functionality as IdleTimeout. If no problems are found with it's usage in the HttpClient in 9.4.x, then IdleTimeout should be retired from usage in the server connections and sessions

[stale[bot]]

This issue has been automatically marked as stale because it has been a full year without activit. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sbordet]

IdleTimeout has a very cheap mechanism to set itself as non-idle, just re-assigning a long field in notIdle(). IdleTimeout looks similar to CyclicTimeouts.Expirable, in that it only needs to maintain the expireNanoTime field as a simple long. In contrast, CyclicTimeout has a more expensive mechanism in schedule() that allocates and performs a CAS. Both will only schedule as task to the Scheduler rarely, typically every idleTimeout interval.

The only class using IdleTimeout is AbstractEndPoint. For a large number of EndPoints, there will be the same number of Scheduler tasks, so it seems natural to use CyclicTimeouts to manage them all with a single Scheduler tasks, and make EndPoint implement CyclicTimeouts.Expirable. However, with such a setup, EndPoint needs to be able to call CyclicTimeout.schedule() if the EndPoint idle timeout is changed, which is not particularly easy, as the EndPoint implementation would need a reference back to the "parent" entity that manages all the EndPoints. It would therefore be difficult to have a complete timeout implementation in the base class, because e.g. SelectableChannelEndPoint would need a reference back to ManagedSelector that manages all the selectable EndPoints, and LocalEndPoint would need a reference back to LocalConnector. Also, it would not be possible to have a "standalone" EndPoint such as ByteArrayEndPoint to manage its own idle timeout, as it would always need a "parent" entity to manage the idle timeout.

All in all, it seems IdleTimeout is actually better than CyclicTimeout for a single entity: it has a much cheaper notIdle() implementation, and equivalent characteristics about scheduling a task to the Scheduler -- a task is only scheduled rarely, every idleTimeout periods.

SessionInactivityTimer will probably benefit from switching from CyclicTimeout to IdleTimeout, as it mostly calls notIdle() IIUC.

CyclicTimeout is better suited for those use cases where you want to tell it "wake up at this time / after this delay" so there is a time/delay value (e.g. Quiche's timeouts, or in the implementation of CyclicTimeouts). IdleTimeout is better suited for those use cases where you want to tell it "not idle" -- there is no time/delay value.

@gregw thoughts?

[gregw]

@sbordet I agree that the complexity of making EndPoint use CyclicTimouts is too much.

But the concern I have is that IdleTimeout and CyclicTimeout (not plural) are very similar and it is not clear which to use and why. If IdleTimeout is more efficient, perhaps we should remove usages of CyclicTimeout in favour of IdleTimeout?