FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fixup node_platform blocking task drain by addaleax · Pull Request #15639 · nodejs/node · GitHub

/ node Public

src: fixup node_platform blocking task drain - #15639

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:platform-fixup
ConversationCommits (1)ChecksFiles changed
Closed

src: fixup node_platform blocking task drain#15639
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:platform-fixup

Conversation

addaleax commented Sep 27, 2017

Copy link
Copy Markdown
Member

#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.

Ref: #15428
Ref: f27b5e4

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src/node_platform

@hashseed @nodejs/v8

src: fixup node_platform blocking task drain
5ae5afe 
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.

Ref: nodejs#15428
Ref: f27b5e4
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 27, 2017

addaleax commented Sep 27, 2017

Copy link
Copy Markdown
Member Author

(If you’re wondering why there’s no test here: Because right now everything still works. 😄 It might be nice for us to get a basic WASM test into our test suite, but that seems like a perfect slightly-more-advanced task for Code & Learn in Vancouver.)

TimothyGu mentioned this pull request Sep 28, 2017
Closed
2 tasks

krisselden commented Sep 28, 2017

Copy link
Copy Markdown

thank you

addaleax mentioned this pull request Sep 28, 2017
Closed

addaleax commented Sep 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/12673/ (edit: should be fine 👍)

addaleax added backport-requested-v8.x lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency. labels Sep 28, 2017
TimothyGu mentioned this pull request Sep 29, 2017
Closed
bnoordhuis approved these changes Sep 29, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but does it matter that this changes the order of foreground vs. background tasks? (I'm guessing no.)

There's a lot of red in the CI. I think it's unrelated but CI again just in case: https://ci.nodejs.org/job/node-test-pull-request/10340/

addaleax commented Sep 29, 2017

Copy link
Copy Markdown
Member Author

LGTM but does it matter that this changes the order of foreground vs. background tasks? (I'm guessing no.)

I agree, that shouldn’t matter – I don’t think V8 can expect any particular timing/ordering of background tasks beyond what is implied by causal relations

BridgeAR commented Oct 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Landed in dcd890a

BridgeAR closed this Oct 1, 2017
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 1, 2017
src: fixup node_platform blocking task drain
dcd890a 
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 deleted the platform-fixup branch October 2, 2017 09:52
addaleax added a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
src: fixup node_platform blocking task drain
e02fc82 
nodejs/node#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/node#15639
Refs: nodejs/node#15428
Refs: f27b5e4bdaafc73a830a0451ee3c641b8bcd08fe
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins commented Oct 7, 2017

Copy link
Copy Markdown
Contributor

quick ping re: backport

should include #15428 and #15691 (once it lands)

addaleax mentioned this pull request Oct 8, 2017
Closed
addaleax added a commit to addaleax/node that referenced this pull request Oct 14, 2017
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>
targos pushed a commit that referenced this pull request Oct 18, 2017
src: fixup node_platform blocking task drain
2572203 
#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: #15639
Refs: #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>
targos pushed a commit that referenced this pull request Oct 18, 2017
src: fixup node_platform blocking task drain
419876a 
#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: #15639
Refs: #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>

MylesBorins commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x?

MylesBorins commented Dec 19, 2017

Copy link
Copy Markdown
Contributor

ping @addaleax

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL