On Wed, 3 Jan 2024 11:11:40 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add select timeout field to the event
>
> src/java.base/share/classes/sun/nio/ch/SelectorImpl.java line 153:
>
>> 151: if ((n == 0) || (SelectorSelectEvent.shouldCommit(duration))) {
>> 152: timeout = (timeout < 0) ? 0 : timeout;
>> 153: SelectorSelectEvent.commit(start, duration, n, timeout);
>
> The comment is a bit confusing here. n == 0 means that no selected keys were
> added or consumed before timeout or wakeup.
Is n == 0 intended to detect a spinning condition where the selector goes back
into select when the event has not been handled?
In that case should we still emit an event if a timeout is present and the
duration is greater than the timeout? For instance, in the HttpClient, we have
a selector thread that will wake up at regular interval - we set up a timeout
which is the min between the next expected timer event and 1500ms. So that
could potentially fire an event every 1500ms if for instance the connection
threshold is less than 1500ms and the connection is idle.
Maybe that's OK - and maybe in that case the onus is on the user to set a
threshold greater than 1500ms?
Or should it be ((n == 0 && durationToMillis(duration) <
timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in
the same unit of time) - also if shouldCommit return false will the event
actually be emitted down the road? Because if not then the ( n== 0 || ...)
won't work.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1502681499
