Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

observed setImmediate and setTimeout execution order contradicts documentation #7145

Closed
tristanls opened this issue Jun 4, 2016 · 14 comments

Comments

Projects
None yet
@tristanls
Copy link
Contributor

commented Jun 4, 2016

  • Version: v6.2.1
  • Platform: Darwin
  • Subsystem: timers

Documentation for setImmediate:

Schedules "immediate" execution of callback after I/O events' callbacks and before timers set by setTimeout and setInterval are triggered.

Demonstration:

console.log("nodejs version", process.versions.node);
console.log("start");
setTimeout(() =>
{
    console.log("setTimeout 0 callback");
}, 0);
setTimeout(() =>
{
    console.log("setTimeout 1 callback");
}, 1);
setImmediate(() =>
{
    console.log("setImmediate callback");
});
setTimeout(() =>
{
    console.log("setTimeout 0 after setImmediate callback");
}, 0);
setTimeout(() =>
{
    console.log("setTimeout 1 after setImmediate callback");
}, 1);
process.nextTick(() => {
  console.log("nextTick callback");
});
console.log("scheduled");

Observed outputs:

$ node test.js 
nodejs version 6.2.1
start
scheduled
nextTick callback
setTimeout 0 callback
setTimeout 1 callback
setImmediate callback
setTimeout 0 after setImmediate callback
setTimeout 1 after setImmediate callback
$ node test.js 
nodejs version 6.2.1
start
scheduled
nextTick callback
setTimeout 0 callback
setTimeout 1 callback
setTimeout 0 after setImmediate callback
setTimeout 1 after setImmediate callback
setImmediate callback

Expected output:

$ node test.js 
nodejs version 6.2.1
start
scheduled
nextTick callback
setImmediate callback
setTimeout 0 callback
setTimeout 1 callback
setTimeout 0 after setImmediate callback
setTimeout 1 after setImmediate callback

@mscdex mscdex added the timers label Jun 4, 2016

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2016

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Jun 5, 2016

I think this is a documentation issue. The documentation is correct about the order of events except at start-up, before node enters the event loop. If you wrap the test case in a setTimeout(), the order of events is as you would expect.

Worthwhile to point out: setTimeout(0) and setTimeout(1) are really the same thing, they both start a timer with a 1 millisecond timeout.

@bnoordhuis bnoordhuis added the doc label Jun 5, 2016

@tristanls

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2016

Thank you for the explanation. I confirmed that wrapping in setTimeout(...) results in expected order.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2016

@tristanls would you like to take a crack at improving the docs?

@tristanls

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2016

@cjihrig I plan on doing so.

@tristanls tristanls referenced this issue Jun 5, 2016

Closed

doc: more detail on when timers are executed #7166

2 of 2 tasks complete
@blacksun1

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

CC: @cjihrig, @tristanls

Hey guys,

I got tripped up on this issue today as well. I see that this documentation update wan't committed and looks like it has been forgotten about or just no one has had time to put it in.

Would you like me to have a crack at redoing this now that #6937 is in place?

Regards,

@blacksun1

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

Wow, OK, I think that this is just broken as per the following

'use strict';

let immediateFired;
let numberOfIterations = 0;

// Handle any errors or rejections

process.on('uncaughtException', (err) => {
  console.error(`uncaughtException handled on iteration ${numberOfIterations}`)
  process.exit(1);
});

console.log(`Running test at ${new Date().toISOString()} on`, process.release, process.arch);

process.on('beforeExit', function() {

  immediateFired = 0;
  numberOfIterations++;
  setTimeout(main, 1);
});

function main() {

  setImmediate(() => {
    
    immediateFired++;
    // console.log('setImmediate 1 fired');
  });

  setTimeout(() => {
    
    if (immediateFired < 2) {
      throw new Error('setTimeout was fired before set immediate');
    }

    // console.log('setTimeout 1');
  }, 1);

  setImmediate(() => {
    
    immediateFired++;
    // console.log('setImmediate 2 fired');
  });
}

Basically, just run this code and my expectation is that it should just keep on running. In fact, what happens is after a random number of iterations it will stop with an error such as

test node simplest.js
Running test at 2017-01-17T05:23:34.918Z on { name: 'node',
  sourceUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0.tar.gz',
  headersUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0-headers.tar.gz' } x64
uncaughtException handled on iteration 32
➜  test node simplest.js
Running test at 2017-01-17T05:23:35.563Z on { name: 'node',
  sourceUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0.tar.gz',
  headersUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0-headers.tar.gz' } x64
uncaughtException handled on iteration 2
➜  test node simplest.js
Running test at 2017-01-17T05:23:36.071Z on { name: 'node',
  sourceUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0.tar.gz',
  headersUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0-headers.tar.gz' } x64
uncaughtException handled on iteration 7
➜  test node simplest.js
Running test at 2017-01-17T05:23:36.532Z on { name: 'node',
  sourceUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0.tar.gz',
  headersUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0-headers.tar.gz' } x64
uncaughtException handled on iteration 66
➜  test node simplest.js
Running test at 2017-01-17T05:23:36.964Z on { name: 'node',
  sourceUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0.tar.gz',
  headersUrl: 'https://nodejs.org/download/release/v7.4.0/node-v7.4.0-headers.tar.gz' } x64
uncaughtException handled on iteration 58

I think that this is probably caused by issue Make setTimeout and setImmediate timers run consecutively #8460.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

@Fishrock123 Fishrock123 self-assigned this Jan 17, 2017

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

I think this is a documentation issue. The documentation is correct about the order of events except at start-up, before node enters the event loop. If you wrap the test case in a setTimeout(), the order of events is as you would expect.

@bnoordhuis That sounds like a bug to me. Is that because we execute startup in nextTick?

#8460 sounds like it may be potentially the actual problem, I probably did not recognize it because there was no testcase.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

@Fishrock123 Same questions here as in #8460 (comment)

@mannanali413

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2017

@Fishrock123 and @Trott does this bug still exist? I can work on this :)

@Trott

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

@Fishrock123 and @Trott does this bug still exist?

Seems like it to me.

@PranayPant

This comment has been minimized.

Copy link

commented Jan 21, 2018

Is this really an issue? nodejs/help#392 explains how that's intended functionality if im not mistaken

@apapirovski

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

The issue reported above by @blacksun1 was fixed in a semver-major commit and will land in 10.x. Unfortunately I don't think it can be back-ported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.