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

worker: spin uv_run twice before closing loop #26138

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@addaleax
Copy link
Member

commented Feb 15, 2019

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
On Windows, the Platform’s `uv_async_t` may need two iterations
before closing when it was previously in use.

Refs: #26089
Refs: #26006

@addaleax addaleax added the worker label Feb 15, 2019

@addaleax addaleax referenced this pull request Feb 15, 2019

Closed

process: start coverage collection before bootstrap #26006

2 of 4 tasks complete
// 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);

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 15, 2019

Member

Should this be in a #ifdef Win32 or similar?

This comment has been minimized.

Copy link
@eugeneo

eugeneo Feb 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@addaleax

addaleax Feb 15, 2019

Author Member

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

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Feb 16, 2019

Member

Perhaps we should keep the behavior consistent then.

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

Landed in 1d51353

@addaleax addaleax closed this Feb 17, 2019

@addaleax addaleax deleted the addaleax:worker-run-twice branch Feb 17, 2019

addaleax added a commit that referenced this pull request Feb 17, 2019

worker: spin uv_run twice before closing loop
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
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 BridgeAR referenced this pull request Feb 26, 2019

Merged

v11.11.0 proposal #26322

rvagg added a commit that referenced this pull request Feb 28, 2019

worker: spin uv_run twice before closing loop
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.