Revisit unrefed handles and beforeExit internals

[Fishrock123]

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

[saghul]

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]

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

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

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

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

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

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

[saghul]

Not yet, sorry :-S

[Trott]

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

[Fishrock123]

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]

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.