Simplify timer with macrotask callback (#4385)

cli/js/globals.ts

@@ -91,6 +91,8 @@ declare global {
       data?: ArrayBufferView
     ): null | Uint8Array;
 
+    setMacrotaskCallback(cb: () => boolean): void;
+
     shared: SharedArrayBuffer;
 
     evalContext(

cli/js/runtime.ts

@@ -7,6 +7,7 @@ import { setBuildInfo } from "./build.ts";
 import { setVersions } from "./version.ts";
 import { setPrepareStackTrace } from "./error_stack.ts";
 import { Start, start as startOp } from "./ops/runtime.ts";
+import { handleTimerMacrotask } from "./web/timers.ts";
 
 export let OPS_CACHE: { [name: string]: number };
 
@@ -27,6 +28,7 @@ export function initOps(): void {
   for (const [name, opId] of Object.entries(OPS_CACHE)) {
     core.setAsyncHandler(opId, getAsyncHandler(name));
   }
+  core.setMacrotaskCallback(handleTimerMacrotask);
 }
 
 export function start(source?: string): Start {

cli/js/tests/timers_test.ts

@@ -127,6 +127,9 @@ unitTest(async function intervalSuccess(): Promise<void> {
   clearInterval(id);
   // count should increment twice
   assertEquals(count, 1);
+  // Similar false async leaking alarm.
+  // Force next round of polling.
+  await waitForMs(0);
 });
 
 unitTest(async function intervalCancelSuccess(): Promise<void> {
@@ -330,24 +333,32 @@ unitTest(async function timerNestedMicrotaskOrdering(): Promise<void> {
   s += "0";
   setTimeout(() => {
     s += "4";
-    setTimeout(() => (s += "8"));
-    Promise.resolve().then(() => {
+    setTimeout(() => (s += "A"));
+    Promise.resolve()
+      .then(() => {
         setTimeout(() => {
-        s += "9";
+          s += "B";
           resolve();
         });
+      })
+      .then(() => {
+        s += "5";
       });
   });
-  setTimeout(() => (s += "5"));
+  setTimeout(() => (s += "6"));
   Promise.resolve().then(() => (s += "2"));
   Promise.resolve().then(() =>
     setTimeout(() => {
-      s += "6";
-      Promise.resolve().then(() => (s += "7"));
+      s += "7";
+      Promise.resolve()
+        .then(() => (s += "8"))
+        .then(() => {
+          s += "9";
+        });
     })
   );
   Promise.resolve().then(() => Promise.resolve().then(() => (s += "3")));
   s += "1";
   await promise;
-  assertEquals(s, "0123456789");
+  assertEquals(s, "0123456789AB");
 });

cli/js/web/timers.ts

@@ -31,8 +31,21 @@ function clearGlobalTimeout(): void {
 
 let pendingEvents = 0;
 const pendingFireTimers: Timer[] = [];
-let hasPendingFireTimers = false;
-let pendingScheduleTimers: Timer[] = [];
+
+/** Process and run a single ready timer macrotask.
+ * This function should be registered through Deno.core.setMacrotaskCallback.
+ * Returns true when all ready macrotasks have been processed, false if more
+ * ready ones are available. The Isolate future would rely on the return value
+ * to repeatedly invoke this function until depletion. Multiple invocations
+ * of this function one at a time ensures newly ready microtasks are processed
+ * before next macrotask timer callback is invoked. */
+export function handleTimerMacrotask(): boolean {
+  if (pendingFireTimers.length > 0) {
+    fire(pendingFireTimers.shift()!);
+    return pendingFireTimers.length === 0;
+  }
+  return true;
+}
 
 async function setGlobalTimeout(due: number, now: number): Promise<void> {
   // Since JS and Rust don't use the same clock, pass the time to rust as a
@@ -54,7 +67,29 @@ async function setGlobalTimeout(due: number, now: number): Promise<void> {
   await startGlobalTimer(timeout);
   pendingEvents--;
   // eslint-disable-next-line @typescript-eslint/no-use-before-define
-  fireTimers();
+  prepareReadyTimers();
+}
+
+function prepareReadyTimers(): void {
+  const now = Date.now();
+  // Bail out if we're not expecting the global timer to fire.
+  if (globalTimeoutDue === null || pendingEvents > 0) {
+    return;
+  }
+  // After firing the timers that are due now, this will hold the first timer
+  // list that hasn't fired yet.
+  let nextDueNode: DueNode | null;
+  while ((nextDueNode = dueTree.min()) !== null && nextDueNode.due <= now) {
+    dueTree.remove(nextDueNode);
+    // Fire all the timers in the list.
+    for (const timer of nextDueNode.timers) {
+      // With the list dropped, the timer is no longer scheduled.
+      timer.scheduled = false;
+      // Place the callback to pending timers to fire.
+      pendingFireTimers.push(timer);
+    }
+  }
+  setOrClearGlobalTimeout(nextDueNode && nextDueNode.due, now);
 }
 
 function setOrClearGlobalTimeout(due: number | null, now: number): void {
@@ -68,15 +103,6 @@ function setOrClearGlobalTimeout(due: number | null, now: number): void {
 function schedule(timer: Timer, now: number): void {
   assert(!timer.scheduled);
   assert(now <= timer.due);
-  // There are more timers pending firing.
-  // We must ensure new timer scheduled after them.
-  // Push them to a queue that would be depleted after last pending fire
-  // timer is fired.
-  // (This also implies behavior of setInterval)
-  if (hasPendingFireTimers) {
-    pendingScheduleTimers.push(timer);
-    return;
-  }
   // Find or create the list of timers that will fire at point-in-time `due`.
   const maybeNewDueNode = { due: timer.due, timers: [] };
   let dueNode = dueTree.find(maybeNewDueNode);
@@ -99,10 +125,6 @@ function unschedule(timer: Timer): void {
   // If either is true, they are not in tree, and their idMap entry
   // will be deleted soon. Remove it from queue.
   let index = -1;
-  if ((index = pendingScheduleTimers.indexOf(timer)) >= 0) {
-    pendingScheduleTimers.splice(index);
-    return;
-  }
   if ((index = pendingFireTimers.indexOf(timer)) >= 0) {
     pendingFireTimers.splice(index);
     return;
@@ -157,57 +179,6 @@ function fire(timer: Timer): void {
   callback();
 }
 
-function fireTimers(): void {
-  const now = Date.now();
-  // Bail out if we're not expecting the global timer to fire.
-  if (globalTimeoutDue === null || pendingEvents > 0) {
-    return;
-  }
-  // After firing the timers that are due now, this will hold the first timer
-  // list that hasn't fired yet.
-  let nextDueNode: DueNode | null;
-  while ((nextDueNode = dueTree.min()) !== null && nextDueNode.due <= now) {
-    dueTree.remove(nextDueNode);
-    // Fire all the timers in the list.
-    for (const timer of nextDueNode.timers) {
-      // With the list dropped, the timer is no longer scheduled.
-      timer.scheduled = false;
-      // Place the callback to pending timers to fire.
-      pendingFireTimers.push(timer);
-    }
-  }
-  if (pendingFireTimers.length > 0) {
-    hasPendingFireTimers = true;
-    // Fire the list of pending timers as a chain of microtasks.
-    globalThis.queueMicrotask(firePendingTimers);
-  } else {
-    setOrClearGlobalTimeout(nextDueNode && nextDueNode.due, now);
-  }
-}
-
-function firePendingTimers(): void {
-  if (pendingFireTimers.length === 0) {
-    // All timer tasks are done.
-    hasPendingFireTimers = false;
-    // Schedule all new timers pushed during previous timer executions
-    const now = Date.now();
-    for (const newTimer of pendingScheduleTimers) {
-      newTimer.due = Math.max(newTimer.due, now);
-      schedule(newTimer, now);
-    }
-    pendingScheduleTimers = [];
-    // Reschedule for next round of timeout.
-    const nextDueNode = dueTree.min();
-    const due = nextDueNode && Math.max(nextDueNode.due, now);
-    setOrClearGlobalTimeout(due, now);
-  } else {
-    // Fire a single timer and allow its children microtasks scheduled first.
-    fire(pendingFireTimers.shift()!);
-    // ...and we schedule next timer after this.
-    globalThis.queueMicrotask(firePendingTimers);
-  }
-}
-
 export type Args = unknown[];
 
 function checkThis(thisArg: unknown): void {

core/bindings.rs

@@ -24,6 +24,9 @@ lazy_static! {
       v8::ExternalReference {
         function: send.map_fn_to()
       },
+      v8::ExternalReference {
+        function: set_macrotask_callback.map_fn_to()
+      },
       v8::ExternalReference {
         function: eval_context.map_fn_to()
       },
@@ -145,6 +148,19 @@ pub fn initialize_context<'s>(
     send_val.into(),
   );
 
+  let mut set_macrotask_callback_tmpl =
+    v8::FunctionTemplate::new(scope, set_macrotask_callback);
+  let set_macrotask_callback_val = set_macrotask_callback_tmpl
+    .get_function(scope, context)
+    .unwrap();
+  core_val.set(
+    context,
+    v8::String::new(scope, "setMacrotaskCallback")
+      .unwrap()
+      .into(),
+    set_macrotask_callback_val.into(),
+  );
+
   let mut eval_context_tmpl = v8::FunctionTemplate::new(scope, eval_context);
   let eval_context_val =
     eval_context_tmpl.get_function(scope, context).unwrap();
@@ -429,6 +445,27 @@ fn send(
   }
 }
 
+fn set_macrotask_callback(
+  scope: v8::FunctionCallbackScope,
+  args: v8::FunctionCallbackArguments,
+  _rv: v8::ReturnValue,
+) {
+  let deno_isolate: &mut Isolate =
+    unsafe { &mut *(scope.isolate().get_data(0) as *mut Isolate) };
+
+  if !deno_isolate.js_macrotask_cb.is_empty() {
+    let msg =
+      v8::String::new(scope, "Deno.core.setMacrotaskCallback already called.")
+        .unwrap();
+    scope.isolate().throw_exception(msg.into());
+    return;
+  }
+
+  let macrotask_cb_fn =
+    v8::Local::<v8::Function>::try_from(args.get(0)).unwrap();
+  deno_isolate.js_macrotask_cb.set(scope, macrotask_cb_fn);
+}
+
 fn eval_context(
   scope: v8::FunctionCallbackScope,
   args: v8::FunctionCallbackArguments,

core/isolate.rs

@@ -166,6 +166,7 @@ pub struct Isolate {
   pub(crate) global_context: v8::Global<v8::Context>,
   pub(crate) shared_ab: v8::Global<v8::SharedArrayBuffer>,
   pub(crate) js_recv_cb: v8::Global<v8::Function>,
+  pub(crate) js_macrotask_cb: v8::Global<v8::Function>,
   pub(crate) pending_promise_exceptions: HashMap<i32, v8::Global<v8::Value>>,
   shared_isolate_handle: Arc<Mutex<Option<*mut v8::Isolate>>>,
   pub(crate) js_error_create_fn: Box<JSErrorCreateFn>,
@@ -299,6 +300,7 @@ impl Isolate {
       pending_promise_exceptions: HashMap::new(),
       shared_ab: v8::Global::<v8::SharedArrayBuffer>::new(),
       js_recv_cb: v8::Global::<v8::Function>::new(),
+      js_macrotask_cb: v8::Global::<v8::Function>::new(),
       snapshot_creator: maybe_snapshot_creator,
       snapshot: load_snapshot,
       has_snapshotted: false,
@@ -495,6 +497,7 @@ impl Future for Isolate {
     let v8_isolate = inner.v8_isolate.as_mut().unwrap();
     let js_error_create_fn = &*inner.js_error_create_fn;
     let js_recv_cb = &inner.js_recv_cb;
+    let js_macrotask_cb = &inner.js_macrotask_cb;
     let pending_promise_exceptions = &mut inner.pending_promise_exceptions;
 
     let mut hs = v8::HandleScope::new(v8_isolate);
@@ -550,6 +553,8 @@ impl Future for Isolate {
       )?;
     }
 
+    drain_macrotasks(scope, js_macrotask_cb, js_error_create_fn)?;
+
     check_promise_exceptions(
       scope,
       pending_promise_exceptions,
@@ -603,6 +608,41 @@ fn async_op_response<'s>(
   }
 }
 
+fn drain_macrotasks<'s>(
+  scope: &mut impl v8::ToLocal<'s>,
+  js_macrotask_cb: &v8::Global<v8::Function>,
+  js_error_create_fn: &JSErrorCreateFn,
+) -> Result<(), ErrBox> {
+  let context = scope.get_current_context().unwrap();
+  let global: v8::Local<v8::Value> = context.global(scope).into();
+  let js_macrotask_cb = js_macrotask_cb.get(scope);
+  if js_macrotask_cb.is_none() {
+    return Ok(());
+  }
+  let js_macrotask_cb = js_macrotask_cb.unwrap();
+
+  // Repeatedly invoke macrotask callback until it returns true (done),
+  // such that ready microtasks would be automatically run before
+  // next macrotask is processed.
+  loop {
+    let mut try_catch = v8::TryCatch::new(scope);
+    let tc = try_catch.enter();
+
+    let is_done = js_macrotask_cb.call(scope, context, global, &[]);
+
+    if let Some(exception) = tc.exception() {
+      return exception_to_err_result(scope, exception, js_error_create_fn);
+    }
+
+    let is_done = is_done.unwrap();
+    if is_done.is_true() {
+      break;
+    }
+  }
+
+  Ok(())
+}
+
 pub(crate) fn attach_handle_to_error(
   scope: &mut impl v8::InIsolate,
   err: ErrBox,