setImmediate regression in v6.8.0

@ErisDS

Version: v6.8.0
Platform: ?
Subsystem: timers

See: TryGhost/Ghost#7555
Moving from: #8655 (comment)

Regression from timers: improve setImmediate() performance (#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

@jasnell

CC reviewers: @jasnell, @imyller.

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

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

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

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...

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

I have a fix coming shortly.

Proposed fix: #9086

timers: fix regression with clearImmediate()

Navigation Menu

Repository navigation

setImmediate regression in v6.8.0 #9084

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

setImmediate regression in v6.8.0 #9084

Description

Activity

jbergstroem commented on Oct 13, 2016

Fishrock123 commented on Oct 13, 2016

targos commented on Oct 13, 2016

Trott commented on Oct 13, 2016

Trott commented on Oct 13, 2016

jasnell commented on Oct 13, 2016

mscdex commented on Oct 13, 2016

mscdex commented on Oct 13, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions