Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 26, 2022

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

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.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 26, 2022
@Arun7897kumar

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants