WIP src: don't check closing handles on beforeExit

[trevnorris]

uv_loop_alive() returns true if there are closing handles. This causes
an issue running the beforeExit event and a timer has been unref'd:

process.on('beforeExit', function() {
setTimeout(function() { }, 100).unref();
});

The event loop also shouldn't run one additional time or else the timer
may execute an additional time.

This is a WIP commit, and mainly opened to raise discussion around the problem at hand.

R=@bnoordhuis

[thlorenz]

Fixes

// before-exit-hangs.js'use strict';process.on('beforeExit',function(){setInterval(function(){},1).unref();});

This one would hang indefinitey

// unrefd-timers-execute-onexit.jsvarassert=require("assert")varintervals=0vari=setInterval(function(){intervals++i.unref()eatTime()},10)functioneatTime(){// the goal of this function is to take longer than the intervalvarcount=0while(count++<1e7){Math.random()}}process.on("exit",function(){assert.equal(intervals,1)})

Still failing

// test-regress-GH-io-1151.jsvarassert=require('assert');setImmediate(function(){require("http");});vari=setInterval(function(){setImmediate(process.exit)},10);i.unref();process.on("exit",onexit);functiononexit(){assert.strictEqual(process._getActiveHandles().length,0,'should have no more active handles on exit')}

// test-timers-unref-interval-exit-after-next.jsvarassert=require('assert');vari=setInterval(function(){// does not matter if we swap the nextTick with the setImmediateprocess.nextTick(function(){setImmediate(process.exit);})},1);i.unref();process.on('exit',function(){assert.strictEqual(process._getActiveHandles().length,0);});

[thlorenz]

I investigated into libuv a bit and found the following oddity:

If you call unref it won't remove handles if they are currently closing due to:
if (((h)->flags & UV__HANDLE_CLOSING) != 0) break;

Then when you call uv_loop_alive it'll return true if handles are closing due to loop->closing_handles != NULL;

So if we go around the loop while one handle is closing we won't exit, but during that loop a new handle could be created.

Not sure if I'm misunderstanding something here or if this could be at least one reason for this issue.

@saghul please correct me if I'm wrong here.

[bnoordhuis]

@thlorenz Your analysis is correct but I wouldn't call it an oddity, it's by design.

Close callbacks must run because that's the only place in the life cycle of a handle where it's safe for the libuv embedder to clean up resources associated with the handle. That's why closing an unref'd handle gives it an implicit ref.

[bnoordhuis]

You're probably well aware of this but active_handles and active_reqs are implementation details.

[trevnorris]

Yeah. That was done just to bring up the issue I'm working around, and the fact I can't figure out how to get around it.

[Fishrock123]

Related to #1151

[Fishrock123]

Still failing

Both of the failing checks check for the leak #1152 was supposed to fix, and not the issue at hand.

@thlorenz Maybe they will work properly if you check the iterations?

[thlorenz]

FWIW with this patch applied you can see from the logs that there is a race condition regarding active/closing handles when running unrefd-timers-execute-onexit.js test.

A timer gets started while the previous one finishes closing.

Excerpt

hasAH: 1, hasAR: 0, hasCH: 0
hasAH: 1, hasAR: 0, hasCH: 0
[ uv_unref ]
[--] -> [--] timer 0x105a00050::8
[ uv_timer_start ]
[--] -> [-A] timer 0x105a00050::10
hasAH: 0, hasAR: 0, hasCH: 1
hasAH: 0, hasAR: 0, hasCH: 1
[ uv_unref ]
[--] -> [--] timer 0x105a00050::10
[ uv_timer_start ]
[R-] -> [RA] timer 0x101f01bf0::11
hasAH: 1, hasAR: 0, hasCH: 0
hasAH: 1, hasAR: 0, hasCH: 0
[--] -> [--] timer 0x105a00050::12

• hasAH: has active handles
• hasAR: has active requests
• hasCH: has closing handles

[thlorenz]

@bnoordhuis I understand now why close callbacks must run (also talked to @trevnorris about this).
However if I can create a timer inside that close callback, I could get myself into an endless loop even if I unref it.

Is that correct? If yes do we need a solution or should we just tell people to not do that?

[bnoordhuis]

That's correct and I'm inclined to say it's more of a documentation issue than a technical issue.

[trevnorris]

After thinking on this, and the implications pointed out by @bnoordhuis with the need to run the uv_close() callback so resources can be properly cleaned up, I agree this is a documentation issue. Closing.