Race condition in node 0.8.x

[lbeschastny]

Hi.

I found a race condition in mock-spawn that affects node 0.8.x users.

Consider the following code:

var mockSpawn = require('mock-spawn')
  , mySpawn = mockSpawn();

mySpawn.setDefault(function(done) {
  this.stdin.on('data', function(chunk) {
    console.log('data:', String(chunk));
  });
  this.stdin.on('end', function() {
    console.log('end!');
    done(0);
  });
});

var proc = mySpawn();
proc.stdin.end('Hello world!');

It should print something like

data: Hello world!
end!

But in node 0.8.x data gets emitted before mySpawn handler function is called, so nothing got printed.

This bug can only be reproduced in node 0.8.x, because of a new streaming API that was introduced in node 0.10.x and took care of things like that.

[gcochard]

Re: #7

It looks like this might fix the issue and not introduce any regressions. It may also fix #4 but I'm not sure. Both are definitely related to the streams API. I will do some research.

[lbeschastny]

It looks like this might fix the issue and not introduce any regressions.

Actually, #7 will introduce a regression, because 0.8.x and 0.10.x have very different streaming APIs.

This bug appeared because you're wrapping runner.call with process.nextTick to make it async, but node.js 0.8.x streams start emitting data immediately. In node 0.8.x world you should either explicitly pause readable stream or synchronously register all 'data' listeners the moment the stream is created, or you'll lose your data otherwise.

This behavior was changed in node 0.10.x. Since 0.10.x all stream are paused by default, until the first 'data' listener is registered.

The fix I introduced in #7 is only good for 0.8.x streams, because calling .resume() always switch the stream into flowing mode, which is not what any 0.10.x user will expect. In 0.10.x it's OK to pass readable stream around without registering any 'data' listeners, because it'll keep all you data safe. But calling .resume() will forcibly switch the stream into flowing mode causing data loses.

This difference in Stream behavior was the main reason why people created modules like readable-stream, which simply mirror one particular Stream implementation in order to guarantee a stable streams base, regardless of what version of node.js you're using.

So, #7 is not general enough and should not be merged, but I can see several ways to fix this bug:

• You could remove process.nextTick and call runner function synchronously (not sure if it won't break something else, though).
• You could take Race condition in node 0.8.x #7 as a base and add some smart check to ensure that .pause() and .resume() will only be called in node.js 0.8.x (e.g. when stream.PassThrough is not defined).
• You could drop build-in Stream and use version-independent implementation (e.g. readable-stream).

[lbeschastny]

As for the regression, here is the code to reproduce it:

var mockSpawn = require('mock-spawn')
  , mySpawn = mockSpawn();

mySpawn.setDefault(function(done) {
  var self = this;
  setTimeout(function() {
    self.stdin.on('data', function(chunk) {
      console.log('data:', String(chunk));
    });
    self.stdin.on('end', function() {
      console.log('end!');
      done(0);
    });
  }, 1000)
});

var proc = mySpawn();
proc.stdin.end('Hello world!');

In node.js 0.10.x this code should print

data: Hello world!
end!

But with #7 it won't print anything.

Note that this code is incompatible with node 0.8.x streams.