Skip to content

[WIP] src: fix integration of Atomics.waitAsync()

Rodrigo Muino Tomonari requested to merge github/fork/joyeecheung/waitasync into main

Previously we unref the timers scheduled by PostDelayedTask() and PostNonNestableDelayedTask(), which had been fine because they were only ever scheduled by GC or logging so it didn't hurt to stop the event loop early and ignore them. But now that Atomics.waitAsync() uses PostNonNestableDelayedTask() to resolve a promise, we should keep the event loop open for it.

From offline discussion with @syg, what happens with a Atomics.waitAsync() with a timeout on an atomics that does not get notified in time is host-defined. For a host like Node.js, it makes sense to keep running until the returned promise resolves with a timed-out value i.e. makes it similar to how setTimeout() would behave.

This fix is not yet ready though, pending issues:

  • When the Atomics are notified in time, V8 doesn't notify the platform that the delayed task posted for the Atomics.waitAsync() timeout is cancelled. From what I can tell, V8 only internally marks that task as cancelled, and when the platform runs the task through task->Run(), V8's Run() implementation of that task sees that it's internally cancelled and skips it. Without knowing that the task is already canceled, the embedder would keep the isolate alive for longer than necessary.
  • Is PostNonNestableDelayedTask() supposed to be thread-safe for this use case (at least for the foreground task runner)? IIUC the V8 header does not specify thread safety for the TaskRunner methods, though the default V8 platform has been implemented with thread safety in mind. Currently this method of the foreground task runner is not thread-safe in Node.js because of the access to the main loop.
  • V8's documentation is not quite clear about whether the isolate must be kept alive when there is a delayed task. Historically delayed tasks are scheduled for GC and other disposable tasks, so Node.js has not been keeping the isolate alive for them, leading to this issue. Should V8 provide more information in the task posted so that the embedder can count on something more reliable than nestability to decide whether the isolate should stay alive for it?

Opened https://bugs.chromium.org/p/v8/issues/detail?id=13238 for the issues above

Merge request reports