loop: avoid double calling the timers in UV_RUN_ONCE mode

[mmomtchev]

This change, which is less invasive that it might appear, tracks when each of the event loop routines actually did any work and double calls the timers only if no other handlers have been called.

It implements a solution to both #3686 (unit test timer_no_double_call) and timer_run_once (PR #41)

[trevnorris]

Left a couple of ideas for simplification.

Though, not sure I'm sold on the idea of not running timers after handling an I/O event. For example:

• I use a uv_prepare_t for gathering metrics (something I do a lot of)
• In the uv_prepare_cb I set a timer for 10ms
• Event loop polls for 9ms before receiving an event
• Processing the event takes 2ms
• The final phase of the loop would have triggered the timer because the poll time + event processing time > timer timeout, but with this change the timer would no longer fire.

[trevnorris]

It should be enough to return whether nevents > 0, and simply return 0 in every location above this.

Should be pretty much the same wherever nevents is used in other locations.

[trevnorris]

Actually, you could remove all the extra code in each uv__io_poll() by caching the uv__loop_metrics_t::metrics::events value at the top of the while loop then comparing it here. For example, at the top set:

uint64_t events_count = uv__get_loop_metrics(loop)->metrics.events

and here just check

events_count == uv__get_loop_metrics(loop)->metrics.events

Would still need to check if timers did any work since they're not counted as events.

[mmomtchev]

@trevnorris

• You set a timer for 10ms
• 9ms pass with no events
• The processing of another timer handler takes 2ms
• What happens?

Well, with the double processing of timers, if the first timer was processed before the polling, yours is processed during the second pass. If not - then it is not. This is the major problem with the current situation - it is not very deterministic.

Otherwise you can probably find some counter-examples, but the most important is that the overall behavior is completely deterministic.

For the timers, either there is a definition that the time is measured relative to the beginning of the processing of the timers queue - either there are as many passes as necessary to process all the timers that might have expired after taking into account the additional time spent in the handlers.

[trevnorris]

@mmomtchev
Looking at your repro case in #3686 (comment), IMO the correct steps for the event loop would be:

• Process timers before entering the while loop, unless called with UV_RUN_ONCE/UV_RUN_NOWAIT.
• Always process the timers at the end of the while loop.

This won't have any change to running with UV_RUN_DEFAULT and will only impact the other two cases if both: an event is waiting in the kernel's event queue when poll is called AND if execution time between uv_timer_start() and when the while loop is entered is longer than the timeout.

How UV_RUN_ONCE executes the loop is at odds with the conceptual event loop. The conceptual event loop actually starts with the call to poll. Hence why uv__metrics_inc_loop_count() is placed before the call to uv__io_poll() and not at the beginning of the while loop. So we need to fudge it a little bit.

Actually, you've got me thinking. I might have a solution that's v1.x compatible. I just tested it and all libuv and node.js tests pass, along with fixing your issue. I'll open a PR with the patch and ping you.

Thanks for investigating this and submitting a PR.

[trevnorris]

Here's the patch: #3927

[mmomtchev]

superseded by #3927