Node: setImmediate regression in v6.8.0

[Fishrock123]

• Version: v6.8.0
• Platform: ?
• Subsystem: timers

See: https://github.com/TryGhost/Ghost/issues/7555
Moving from: https://github.com/nodejs/node/pull/8655#issuecomment-253453520

Regression from timers: improve setImmediate() performance (https://github.com/nodejs/node/pull/8655)

Hey there!

We started having test failures due to this change. There's more information on the issue & PR linked just above this comment. To pull some of that here:

The last job never get's executed, because setImmediate does not get triggered!

We have a temporary fix in place in the PR, that reduces the timeout and this appears to work (the tests pass). Would love a little bit of input into how this change has impacted the functionality and whether we've found a bug or are doing something wrong :)

var jobs = [Date.now() + 1000, Date.now() + 2000, Date.now() + 3000];

jobs.forEach(function(timestamp) {
    var timeout = setTimeout(function() {
        clearTimeout(timeout);

        (function retry() {
            var immediate = setImmediate(function() {
                clearImmediate(immediate);

                if (Date.now() < timestamp) {
                    return retry();
                }

                console.log("FINISHED JOB");
            });
        }());
    }, timestamp - 200);
});

v6.8.0 - does not work, you will see 1 x FINISHED JOB
v4.4.7 && v6.7.0 - works as expected, you will see 3 x FINISHED JOB

cc @erisds, @TheAlphaNerd, @mscdex, @trott, @kirrg001

[jbergstroem]

CC reviewers: @jasnell, @imyller.

[Fishrock123]

Smells like a problem with the new linkedlist bits but I'm having a hard time pinpointing it from the diff.

[targos]

Hint: it works correctly if the clearImmediate(immediate); line is removed.

[Trott]

This looks like it could be related to the re-ordering of timers and immediates.

Take this code:

setTimeout(() => {console.log('foo')}, 1);
setImmediate(() => {console.log('bar')});
setTimeout(() => {console.log('foo')}, 1);
setImmediate(() => {console.log('bar')});

In 6.7.0, it usually (but not always!) returns:

bar
bar
foo
foo

But in 6.8.0, it usually (but not always!) returns:

foo
foo
bar
bar

[Trott]

Docs say setImmediate() should fire before timers, so if I'm understanding correctly, the typical results in 6.8.0 are a bug, albeit a bug that sometimes shows up in 6.7.0?

"if I'm understanding correctly" may be a big assumption here...

[jasnell]

Let's revert the change and pinpoint the issue after.

[mscdex]

I have a fix coming shortly.

[mscdex]

Proposed fix: https://github.com/nodejs/node/pull/9086