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

Revisit unrefed handles and beforeExit internals #3665

Closed
Fishrock123 opened this issue Nov 4, 2015 · 11 comments
Closed

Revisit unrefed handles and beforeExit internals #3665

Fishrock123 opened this issue Nov 4, 2015 · 11 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. lib / src Issues and PRs related to general changes in the lib or src directory. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@Fishrock123
Copy link
Contributor

The original issue is at #1264

The synopsis is that because of how uv_run works, unrefed handles show up as active still for the first run, and don't appear as no-longer active until the second run.

In the past, running an unrefed timer in beforeExit would infinitely loop, since the second run would always be in the next beforeExit, consequentially calling 'beforeExit' again and scheduling another time, looping infinitely. This was fixed in #3407 by making unrefed timers of the same timeout use the previous handle, which is then properly unreferenced.

As described, one would expect the event loop / beforeExit code to look something like so:

bool more;
uv_run_mode run_mode = UV_RUN_ONCE;
do {

  more = uv_run(env->event_loop(), run_mode);
  if (more == false) {
    EmitBeforeExit(env);

    // Emit `beforeExit` if the loop became alive either after emitting
    // event, or after running some callbacks.
    more = uv_loop_alive(env->event_loop());
    run_mode = UV_RUN_NOWAIT;
  } else {
    run_mode = UV_RUN_ONCE;
  }
} while (more == true);

However in reality it looks more like this:
(

node/src/node.cc

Lines 4063 to 4078 in 471aa5a

bool more;
do {
v8::platform::PumpMessageLoop(default_platform, isolate);
more = uv_run(env->event_loop(), UV_RUN_ONCE);
if (more == false) {
v8::platform::PumpMessageLoop(default_platform, isolate);
EmitBeforeExit(env);
// Emit `beforeExit` if the loop became alive either after emitting
// event, or after running some callbacks.
more = uv_loop_alive(env->event_loop());
if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0)
more = true;
}
} while (more == true);
)

bool more;
do {
  more = uv_run(env->event_loop(), UV_RUN_ONCE);

  if (more == false) {
    EmitBeforeExit(env);

    // Emit `beforeExit` if the loop became alive either after emitting
    // event, or after running some callbacks.
    more = uv_loop_alive(env->event_loop());
    if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0)
      more = true;
  }
} while (more == true);

If you look closely at the actual version, you'll notice that uv_run ends up actually being called at least 2 times on a beforeExit re-entry anyways, which logically register the timer and unref. However it does not seem to work like that.

Sniff test says it may be some discrepancy within uv_run modes?

cc @indutny, @trevnorris, @saghul

@Fishrock123 Fishrock123 added libuv Issues and PRs related to the libuv dependency or the uv binding. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 4, 2015
@saghul
Copy link
Member

saghul commented Nov 4, 2015

A libuv test which fails on the offending platform would help here. I quickly check when we run the close callbacks and we do it at the same point in time on Unix and Windows, so the issue might be a little non-obvious.

@Fishrock123
Copy link
Contributor Author

@saghul This isn't about a specific platform. Trying to fix the (patched in JavaScript ala #3407) bug exposed something that was, but this is questioning if something deeper isn't wrong. Either that or that we all deeply misunderstand something.

Since the old problems existed independently of platform (as far as I know) and that the current code works ok on all platforms suggests this isn't platform specific.

@trevnorris
Copy link
Contributor

@Fishrock123 The fix we had of walking all active handles and checking the active handle count was different on unix than it was windows.

@Fishrock123
Copy link
Contributor Author

@trevnorris right, I'm pointing out that logically the original failure should never have happened; two+ beforeExit's should not have fired because uv_run runs twice in that situation always and the second time (in the regular loop part) should catch that there aren't active handles anymore and cause it to exit the loop.

@saghul
Copy link
Member

saghul commented Nov 18, 2015

@Fishrock123 sorry, this slipped somewhere to the side on my inbox. So, in a nutshell, reverting the fix in #3407 the test case will fail but it shouldn't, right? If so I'll take a look.

@Fishrock123
Copy link
Contributor Author

@saghul If I understand the logic correctly as laid out in my OP, yes. Again, smells like a uv_run mode discrepancy but I'm not too sure.

@Fishrock123
Copy link
Contributor Author

@saghul were you ever able to take a look here? :)

@saghul
Copy link
Member

saghul commented Jan 11, 2016

Not yet, sorry :-S

@Trott
Copy link
Member

Trott commented Apr 12, 2017

This conversation has been dormant for over a year. Is this something that should be closed?

@Fishrock123
Copy link
Contributor Author

Probably not. Someone should look into it in more detail at some point. We're working around it but the behavior is suspicious, though not outright buggy.

@Fishrock123
Copy link
Contributor Author

On second thought, maybe it would be possible to craft a known_issues test, so we might actually be able to make some bit of progress.

@Trott Trott added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 2, 2017
addaleax pushed a commit that referenced this issue Aug 17, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
Node.js currently uses the V8 implementation of the DefaultPlatform
which schedules VM tasks on a V8 managed thread pool. Since the Node.js
event loop is not aware of these tasks, the Node.js process may exit
while there are outstanding VM tasks. This will become problematic once
asynchronous wasm compilation lands in V8.

This PR introduces a Node.js specific implementation of the v8::Platform
on top of libuv so that the event loop is aware of outstanding VM tasks.

PR-URL: #14001
Fixes: #3665
Fixes: #8496
Fixes: #12980
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. lib / src Issues and PRs related to general changes in the lib or src directory. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

No branches or pull requests

4 participants