unref'd timers running in beforeExit time

[brycebaril]

When working out this test case for #1152 I came up with this:

var assert = require("assert")
var intervals = 0
var i = setInterval(function () {
  intervals++
  i.unref()
  eatTime()
}, 10)

function eatTime() {
  // the goal of this function is to take longer than 10ms, i.e. longer than the interval
  var count = 0
  while (count++ < 1e7) {
    Math.random()
  }
}

process.on("exit", function () {
  assert.equal(intervals, 1)
})

Note -- this requires the patch in #1152 to even terminate, or running Node.js 0.12+ which already has that fix.

I think what happens here is the blocking time causes the repeat time to trigger and the unreferenced timer gets executed in the beforeExit timeframe (nodejs/node-v0.x-archive@a2eeb43) when it shouldn't be. This can result in your application not terminating when it should, and functions that should not be executed being run.

This is a separate bug than #1151 (with fix #1152) and is not fixed by #1231

[Fishrock123]

Is this actually an issue? An unref'd timer isn't guaranteed to execute any number of times without something else holding the thread open.

The loop holds it open, but the number of intervals doesn't mean anything.

My other tests check for still-active handles. If the process exits and there are still some, there is a leak.

process.on('exit', function() {
  assert.strictEqual(process._getActiveHandles().length, 0);
});

[Fishrock123]

@kenansulayman pointed out to me that is not the case, because you are expecting one and the unref'd timer is keeping it open.

I'll try to make a better test.

[trevnorris]

@Fishrock123 Here's a test case that should exit immediately but stays open indefinitely:

process.on('beforeExit', function() {
  setInterval(function() {}, 1).unref();
});

Does the same with setTimeout().

/cc @bnoordhuis

[Fishrock123]

@trevnorris oh. that doesn't seem good. Also, #1152 doesn't fix that. :(

Edit: In addition, it still says there's no active handles for that.

[bnoordhuis]

Probably not worth spending too much time on. I'm in the process of rewriting the timers module and that should fix a host of issues in one swoop. More test cases would be appreciated though. If you file pull requests, I'll subsume them in the final PR.

[Fishrock123]

@brycebaril this test exhibits the same behaviour as the original: #1152 (comment)

I'm still confused as to why adding just the function makes any difference at all for intervals; everything still points to the timeout the same...

[brycebaril]

@Fishrock123 The wait is required because the interval has to be run to leak the handle. It turns out it doesn't even need to be blocking time, E.g.

setTimeout(function () { /* never run */ }, 10000)
setInterval(setImmediate, 100, process.exit).unref()
process.on('exit', function () { console.log(process._getActiveHandles()) })

Will also leak the setImmediate handle.

Likewise for this one the cpu-burn-loop is required to make it something that would have been run during beforeExit if it wasn't already unreferenced. Then by making each run take long enough it would run again, we can make it get run again.

This is also why these two issues are so intertwined -- triggering this issue without the #1152 fix will leak the handles indefinitely. Leaking the handles will increase the chance of this issue.

@bnoordhuis Any guess on a timeframe for the rewrite? We're running into timer issues with io.js in real applications now, holding us back.

[bnoordhuis]

It's a pretty big change. Even if I finish it tomorrow, it's probably going to take some time getting reviewed.