Skip to content

stream: fix sync callback leak

Rodrigo Muino Tomonari requested to merge github/fork/nxtedition/stream-sync-leak into master

This PR currently is mostly a suggestion to discuss related to https://github.com/nodejs/node/pull/31756. It's not properly cleaned up and is mostly to illustrate the problem and possible solution.

Today when working with callbacks and events we need to keep in mind ordering and whether or not a callback is invoked synchronously or asynchronously.

The current pattern for this is something like:

Stream.write = function (chunk) {
  state.sync = true;
  this._write(chunk, this._onWriteComplete);
  state.sync = false;
}

Which mostly works fine. However, as in https://github.com/nodejs/node/pull/31756, if the callback is leaked outside it can still be invoked in the same tick breaking the assumption that when state.sync === false a tick has elapsed.

i.e.


const s = new Stream();
let cb;
s._write = (chunk, callback) => {
  cb = callback();
};
s.write('asd');
cb(); // uhoh! sync but the `Stream` thinks it's async.
s.on('someevent', common.mustCall()); // not called

This PR tries to resolve this by keeping track of which tick we are currently in and making sure that state.sync === false always is a different tick.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Merge request reports