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

worker: spin uv_run twice before closing loop by addaleax · Pull Request #26138 · nodejs/node · GitHub

/ node Public

worker: spin uv_run twice before closing loop - #26138

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:worker-run-twice
ConversationCommits (1)ChecksFiles changed
Closed

worker: spin uv_run twice before closing loop#26138
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:worker-run-twice

Conversation

addaleax commented Feb 15, 2019

Copy link
Copy Markdown
Member

On Windows, the Platform’s uv_async_t may need two iterations
before closing when it was previously in use.

Refs: #26089
Refs: #26006

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

worker: spin uv_run twice before closing loop
06e09f7 
On Windows, the Platform’s `uv_async_t` may need two iterations
before closing when it was previously in use.

Refs: nodejs#26089
Refs: nodejs#26006
addaleax added the worker Issues and PRs related to Worker support. label Feb 15, 2019

nodejs-github-bot commented Feb 15, 2019

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2625/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 15, 2019
addaleax mentioned this pull request Feb 15, 2019
Closed
4 tasks
Fishrock123 reviewed Feb 15, 2019
View reviewed changes
Comment thread src/node_worker.cc
// Need to run the loop twice more to close the platform's uv_async_t
// TODO(addaleax): It would be better for the platform itself to provide
// some kind of notification when it has fully cleaned up.
uv_run(&loop_, UV_RUN_ONCE);

Fishrock123 Feb 15, 2019

Copy link
Copy Markdown
Contributor

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

Should this be in a #ifdef Win32 or similar?

eugeneo Feb 15, 2019

Copy link
Copy Markdown
Contributor

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

Why not UV_RUN_DEFAULT? My concern is that some platform-specific libuv change may at some point require a different arbitrary number or runs.

addaleax Feb 15, 2019

Copy link
Copy Markdown
Member Author

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

@eugeneo Because we want to not keep spinning the event loop indefinitely, which could happen if e.g. addons still have active handles (which we currently crash for, which is what we want).

I agree that this is hacky – hence the TODO – but I wouldn’t expect libuv to change anything here (and if they do, we’d notice because they run Node.js in the libuv CI).

@Fishrock123 I don’t have strong feelings about that, I can add it if you want.

Fishrock123 Feb 16, 2019

Copy link
Copy Markdown
Contributor

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

Perhaps we should keep the behavior consistent then.

addaleax commented Feb 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/20802/ (:heavy_check_mark:)

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 16, 2019

addaleax commented Feb 17, 2019

Copy link
Copy Markdown
Member Author

Landed in 1d51353

addaleax closed this Feb 17, 2019
addaleax deleted the worker-run-twice branch February 17, 2019 23:07
addaleax added a commit that referenced this pull request Feb 17, 2019
worker: spin uv_run twice before closing loop
1d51353 
On Windows, the Platform’s `uv_async_t` may need two iterations
before closing when it was previously in use.

Refs: #26089
Refs: #26006

PR-URL: #26138
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 18, 2019
worker: spin uv_run twice before closing loop
570dbb9 
On Windows, the Platform’s `uv_async_t` may need two iterations
before closing when it was previously in use.

Refs: #26089
Refs: #26006

PR-URL: #26138
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
worker: spin uv_run twice before closing loop
dab64bb 
On Windows, the Platform’s `uv_async_t` may need two iterations
before closing when it was previously in use.

Refs: #26089
Refs: #26006

PR-URL: #26138
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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

joyeecheung joyeecheung approved these changes
+2 more reviewers
Fishrock123 Fishrock123 left review comments
eugeneo eugeneo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL