sinon/lolex fake timers broken when used with this

[kolodny]

Consider the following mocha test file

var lolex = require('lolex');
var through = require('through2');

describe('a bug', function() {
  var clock;
  before(function() {
    clock = lolex.install({toFake: [ 'setImmediate', 'nextTick' ]});
  });
  after(function() {
    clock.uninstall();
  });

  it('does not work well with lolex', function(done) {
    var stream = through.obj();
    stream.on('data', function() {
      stream.on('end', function() {

        console.log('creating stream2')
        var stream2 = through.obj();
        stream2.on('data', function() {
          stream2.on('end', function() {
            console.log('OK')
            done();            
          });
          setImmediate(function() {
            stream2.end()
          });
        });
        stream2.emit('data')

      });
      stream.end();
    });
    stream.emit('data');

    clock.runAll()
  });

});

When the clock calls are removed and this runs using regular time this runs and finishes as expected and without errors. However when running with lolex installed it just hangs after logging creating stream2. This is due to https://github.com/nodejs/readable-stream/blob/d6c391d7101946c8b8b97914fc46fd3322c450d1/lib/_stream_readable.js#L983 which retains a reference to the non swapped out version of process.nextTick due to use of process-nextick-args https://github.com/nodejs/readable-stream/blob/d6c391d7101946c8b8b97914fc46fd3322c450d1/lib/_stream_readable.js#L26

A simple solution can be swapping require('process-nextick-args') with require('./process-nextick-args') which can look something like this:

//  ./process-nextick-args
var nextTickNeedsPatching = (
  !process.version ||
  process.version.indexOf('v0.') === 0 ||
  process.version.indexOf('v1.') === 0 && process.version.indexOf('v1.8.') !== 0
);

var processNextTick = function(fn, a, b, c) {
  return !nextTickNeedsPatching ? process.nextTick(fn, a, b, c) :
  process.nextTick(function() {
    fn(a, b, c);
  });

}

Does that sound like a reasonable solution?

[kolodny]

lolex is the module powering sinon fake timers so this issue affects anyone using sinon fake timers too.

[calvinmetcalf]

you probably want to open something on process-nextick-args

On Mon, Dec 4, 2017 at 11:19 PM Moshe Kolodny notifications@github.com wrote:

lolex is the module powering sinon fake timers so this issue affects anyone using sinon fake timers too.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/readable-stream/issues/316#issuecomment-349190828, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4n7r-YZjpNEL7Ag4qdP9aFCkc5Fh3ks5s9MRCgaJpZM4Q1tTY .

[kolodny]

Good call. https://github.com/calvinmetcalf/process-nextick-args/pull/10

[calvinmetcalf]

reopening as we now need to update process.nextick-args

[benjamingr]

Hey, I can fix this from the lolex side (I wrote the nextTick part there) - is this still an issue?

[benjamingr]

Note lolex doesn't override microtask stuff by default.

[kolodny]

Yeah, this is still an issue. I resorted to patching the relevant files until the nextTick fixes makes it's way to node, but fixing it in lolex would be better. I didn't realize that was an option

[benjamingr]

I'm not sure I understand the issue completely - but I'm willing to patch it on either side (lolex or Node.js). How would a fix from the lolex side look like?

[kolodny]

I'm not sure how to go about a fix from the lolex side, I think that readable-stream needs to update the dependency of process-nextick-args