win,unix: change execution order of timers (#3927)

trevnorris · mmomtchev · web-flow · commit 66009549067c · 2023-03-20T10:04:57.000-06:00
The maximum number of times timers should run when uv_run() is called with UV_RUN_ONCE and UV_RUN_NOWAIT is 1. Do that by conditionally calling timers before entering the while loop when called with UV_RUN_DEFAULT. The reason to always run timers at the end of the while loop, instead of at the beginning, is to help enforce the conceptual event loop model. Which starts when entering the event provider (e.g. calling poll). Other than only allowing timers to be processed once per uv_run() execution, the only other noticeable change this will show is if all the following are true: * uv_run() is called with UV_RUN_NOWAIT or UV_RUN_ONCE. * An event is waiting to be received when poll is called. * Execution time between the call to uv_timer_start() and entering the while loop is longer than the timeout. If all these are true, then timers that would have executed before entering the event provider will now be executed afterward. Fixes: #3686 Co-authored-by: Momtchil Momtchev <momtchil@momtchev.com>
diff --git a/docs/src/design.rst b/docs/src/design.rst
@@ -60,16 +60,15 @@ stages of a loop iteration:
     :align: center
 
 
-#. The loop concept of 'now' is updated. The event loop caches the current time at the start of
-   the event loop tick in order to reduce the number of time-related system calls.
+#. The loop concept of 'now' is initially set.
+
+#. Due timers are run if the loop was run with ``UV_RUN_DEFAULT``. All active timers scheduled
+   for a time before the loop's concept of *now* get their callbacks called.
 
 #. If the loop is *alive*  an iteration is started, otherwise the loop will exit immediately. So,
    when is a loop considered to be *alive*? If a loop has active and ref'd handles, active
    requests or closing handles it's considered to be *alive*.
 
-#. Due timers are run. All active timers scheduled for a time before the loop's concept of *now*
-   get their callbacks called.
-
 #. Pending callbacks are called. All I/O callbacks are called right after polling for I/O, for the
    most part. There are cases, however, in which calling such a callback is deferred for the next
    loop iteration. If the previous iteration deferred any I/O callback it will be run at this point.
@@ -101,9 +100,11 @@ stages of a loop iteration:
 #. Close callbacks are called. If a handle was closed by calling :c:func:`uv_close` it will
    get the close callback called.
 
-#. Special case in case the loop was run with ``UV_RUN_ONCE``, as it implies forward progress.
-   It's possible that no I/O callbacks were fired after blocking for I/O, but some time has passed
-   so there might be timers which are due, those timers get their callbacks called.
+#. The loop concept of 'now' is updated.
+
+#. Due timers are run. Note that 'now' is not updated again until the next loop iteration.
+   So if a timer became due while other timers were being processed, it won't be run until
+   the following event loop iteration.
 
 #. Iteration ends. If the loop was run with ``UV_RUN_NOWAIT`` or ``UV_RUN_ONCE`` modes the
    iteration ends and :c:func:`uv_run` will return. If the loop was run with ``UV_RUN_DEFAULT``
diff --git a/docs/src/static/loop_iteration.png b/docs/src/static/loop_iteration.png
diff --git a/src/unix/core.c b/src/unix/core.c
@@ -390,10 +390,17 @@ int uv_run(uv_loop_t* loop, uv_run_mode mode) {
   if (!r)
     uv__update_time(loop);
 
-  while (r != 0 && loop->stop_flag == 0) {
-    uv__update_time(loop);
+  /* Maintain backwards compatibility by processing timers before entering the
+   * while loop for UV_RUN_DEFAULT. Otherwise timers only need to be executed
+   * once, which should be done after polling in order to maintain proper
+   * execution order of the conceptual event loop. */
+  if (mode == UV_RUN_DEFAULT) {
+    if (r)
+      uv__update_time(loop);
     uv__run_timers(loop);
+  }
 
+  while (r != 0 && loop->stop_flag == 0) {
     can_sleep =
         QUEUE_EMPTY(&loop->pending_queue) && QUEUE_EMPTY(&loop->idle_handles);
 
@@ -424,18 +431,8 @@ int uv_run(uv_loop_t* loop, uv_run_mode mode) {
     uv__run_check(loop);
     uv__run_closing_handles(loop);
 
-    if (mode == UV_RUN_ONCE) {
-      /* UV_RUN_ONCE implies forward progress: at least one callback must have
-       * been invoked when it returns. uv__io_poll() can return without doing
-       * I/O (meaning: no callbacks) when its timeout expires - which means we
-       * have pending timers that satisfy the forward progress constraint.
-       *
-       * UV_RUN_NOWAIT makes no guarantees about progress so it's omitted from
-       * the check.
-       */
-      uv__update_time(loop);
-      uv__run_timers(loop);
-    }
+    uv__update_time(loop);
+    uv__run_timers(loop);
 
     r = uv__loop_alive(loop);
     if (mode == UV_RUN_ONCE || mode == UV_RUN_NOWAIT)
diff --git a/src/win/core.c b/src/win/core.c
@@ -609,10 +609,17 @@ int uv_run(uv_loop_t *loop, uv_run_mode mode) {
   if (!r)
     uv_update_time(loop);
 
-  while (r != 0 && loop->stop_flag == 0) {
-    uv_update_time(loop);
+  /* Maintain backwards compatibility by processing timers before entering the
+   * while loop for UV_RUN_DEFAULT. Otherwise timers only need to be executed
+   * once, which should be done after polling in order to maintain proper
+   * execution order of the conceptual event loop. */
+  if (mode == UV_RUN_DEFAULT) {
+    if (r)
+      uv_update_time(loop);
     uv__run_timers(loop);
+  }
 
+  while (r != 0 && loop->stop_flag == 0) {
     can_sleep = loop->pending_reqs_tail == NULL && loop->idle_handles == NULL;
 
     uv__process_reqs(loop);
@@ -645,18 +652,8 @@ int uv_run(uv_loop_t *loop, uv_run_mode mode) {
     uv__check_invoke(loop);
     uv__process_endgames(loop);
 
-    if (mode == UV_RUN_ONCE) {
-      /* UV_RUN_ONCE implies forward progress: at least one callback must have
-       * been invoked when it returns. uv__io_poll() can return without doing
-       * I/O (meaning: no callbacks) when its timeout expires - which means we
-       * have pending timers that satisfy the forward progress constraint.
-       *
-       * UV_RUN_NOWAIT makes no guarantees about progress so it's omitted from
-       * the check.
-       */
-      uv_update_time(loop);
-      uv__run_timers(loop);
-    }
+    uv_update_time(loop);
+    uv__run_timers(loop);
 
     r = uv__loop_alive(loop);
     if (mode == UV_RUN_ONCE || mode == UV_RUN_NOWAIT)
diff --git a/test/test-list.h b/test/test-list.h
@@ -233,6 +233,8 @@ TEST_DECLARE   (timer_from_check)
 TEST_DECLARE   (timer_is_closing)
 TEST_DECLARE   (timer_null_callback)
 TEST_DECLARE   (timer_early_check)
+TEST_DECLARE   (timer_no_double_call_once)
+TEST_DECLARE   (timer_no_double_call_nowait)
 TEST_DECLARE   (idle_starvation)
 TEST_DECLARE   (idle_check)
 TEST_DECLARE   (loop_handles)
@@ -842,6 +844,8 @@ TASK_LIST_START
   TEST_ENTRY  (timer_is_closing)
   TEST_ENTRY  (timer_null_callback)
   TEST_ENTRY  (timer_early_check)
+  TEST_ENTRY  (timer_no_double_call_once)
+  TEST_ENTRY  (timer_no_double_call_nowait)
 
   TEST_ENTRY  (idle_starvation)
   TEST_ENTRY  (idle_check)
diff --git a/test/test-timer.c b/test/test-timer.c
@@ -30,6 +30,7 @@ static int twice_close_cb_called = 0;
 static int repeat_cb_called = 0;
 static int repeat_close_cb_called = 0;
 static int order_cb_called = 0;
+static int timer_check_double_call_called = 0;
 static uint64_t start_time;
 static uv_timer_t tiny_timer;
 static uv_timer_t huge_timer1;
@@ -368,3 +369,41 @@ TEST_IMPL(timer_early_check) {
   MAKE_VALGRIND_HAPPY(uv_default_loop());
   return 0;
 }
+
+static void timer_check_double_call(uv_timer_t* handle) {
+  timer_check_double_call_called++;
+}
+
+TEST_IMPL(timer_no_double_call_once) {
+  uv_timer_t timer_handle;
+  const uint64_t timeout_ms = 10;
+
+  ASSERT_EQ(0, uv_timer_init(uv_default_loop(), &timer_handle));
+  ASSERT_EQ(0, uv_timer_start(&timer_handle,
+                              timer_check_double_call,
+                              timeout_ms,
+                              timeout_ms));
+  uv_sleep(timeout_ms * 2);
+  ASSERT_EQ(1, uv_run(uv_default_loop(), UV_RUN_ONCE));
+  ASSERT_EQ(1, timer_check_double_call_called);
+
+  MAKE_VALGRIND_HAPPY(uv_default_loop());
+  return 0;
+}
+
+TEST_IMPL(timer_no_double_call_nowait) {
+  uv_timer_t timer_handle;
+  const uint64_t timeout_ms = 10;
+
+  ASSERT_EQ(0, uv_timer_init(uv_default_loop(), &timer_handle));
+  ASSERT_EQ(0, uv_timer_start(&timer_handle,
+                              timer_check_double_call,
+                              timeout_ms,
+                              timeout_ms));
+  uv_sleep(timeout_ms * 2);
+  ASSERT_EQ(1, uv_run(uv_default_loop(), UV_RUN_NOWAIT));
+  ASSERT_EQ(1, timer_check_double_call_called);
+
+  MAKE_VALGRIND_HAPPY(uv_default_loop());
+  return 0;
+}
