Skip to content

[v8.x] src: prepare platform for upstream V8 changes #16091

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[v8.x] src: prepare platform for upstream V8 changes #16091

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 8, 2017 •
edited
Loading

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8.x labels Oct 8, 2017
@addaleax
src: prepare platform for upstream V8 changes
4437df3 
V8 platform tasks may schedule other tasks (both background and
foreground), and may perform asynchronous operations like
resolving Promises.

To address that:

- Run the task queue drain call inside a callback scope.
  This makes sure asynchronous operations inside it, like
  resolving promises, lead to the microtask queue and any
  subsequent operations not being silently forgotten.
- Move the task queue drain call before `EmitBeforeExit()`
  and only run `EmitBeforeExit()` if there is no new event
  loop work.
- Account for possible new foreground tasks scheduled by
  background tasks in `DrainBackgroundTasks()`.

PR-URL: nodejs#15428
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matthew Loring <mattloring@google.com>
@addaleax
src: fixup node_platform blocking task drain
dae892d 
nodejs#15428 was supposed to account
for upcoming changes in V8 upstream, but while addressing review
comments a bug was introduced; `DrainBackgroundTasks()` should
always at least perform one blocking drain on the background task
queue.

PR-URL: nodejs#15639
Refs: nodejs#15428
Refs: f27b5e4
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@addaleax
src: use more appropriate context-entered check
89988dd 
Make the context check in `MakeCallback` match what the comment says
(and what actually makes sense).

PR-URL: nodejs#15691
Fixes: nodejs#15672
Ref: nodejs#15428
Ref: f27b5e4
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@targos
Copy link
Member

targos commented Oct 18, 2017 •
edited
Loading

Landed in fcd9ac9, 2572203 and 6a6173b.

@targos targos closed this Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addaleax @targos @TimothyGu @nodejs-github-bot