sinonjs / sinon

Test spies, stubs and mocks for JavaScript.
https://sinonjs.org/
Other
9.67k stars 769 forks source link

wrong setImmediate simulation #595

Closed arestov closed 10 years ago

arestov commented 10 years ago

callback for setImmediate() must be called before callbacks for setTimeout()

here is simple case for nodejs REPL

setTimeout(function(){console.log('usual', 'must be last')},0);
setImmediate(function(){console.log('usual', 'must be first')});

var sinon = require('sinon');
(function(){
  var clock = sinon.useFakeTimers();
  setTimeout(function(){console.log('simulated', 'must be last')},0);
  setImmediate(function(){console.log('simulated', 'must be first')});

  clock.tick(10);
  clock.restore();
})();

and you will get

usual must be first
usual must be last

simulated must be last
simulated must be first

So there is must be special stack for setImmediate callbacks, which must be called before any callback for setTimeout

duncanbeevers commented 10 years ago

I don't think it's important to match this behavior exactly at this point. Not only is setImmediate non-standard, but none of the literature I was able to find referenced this strict ordering between setImmediate and setTimeout.

Stylistically, I would consider any code that relied on this strict ordering to be suspect, at best, and unacceptably subtle and complex in codebases where I served as gatekeeper.

If there's something I'm missing or reasonable standards showing why this ordering should be enforced, please do explain.

arestov commented 10 years ago

This is exactly what expected from setImmidiate

The main reason to use setImmediate instead of setTimeout(..,0) is that it does not block IO as setTimeout but executes faster (as purposed by microsoft - suggestor of setImmidiate) http://ie.microsoft.com/TEStdrive/Performance/setImmediateSorting/Default.html

setTimeout(fn, 0) can potentially be used, however as it is clamped to 4ms for timers nested more than 5 deep per the HTML spec https://developer.mozilla.org/en-US/docs/Web/API/Window.setImmediate https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/setImmediate/Overview.html#introduction

Information above mostly related to browsers events loop, but nodejs has same behavior http://nodejs.org/api/timers.html

setImmediate(callback, [arg], [...])# To schedule the "immediate" execution of callback after I/O events callbacks and before setTimeout and setInterval .

duncanbeevers commented 10 years ago

@arestov Thanks for the feedback. Take a look over the implemented fix and let me know if it addresses your concerns.

cjohansen commented 10 years ago

Fixed in 1.12