stream: fix sync callback leak

[ronag]

This PR currently is mostly a suggestion to discuss related to #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 #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

[mcollina]

Unfortunately, this approach would make adding this feature extremely hard in readable-stream. I’m -1.

[ronag]

Unfortunately, this approach would make adding this feature extremely hard in readable-stream. I’m -1.

The only other way I can think of is to fallback to always using nextTick which would have significant performance implications.

Should we go the documentation route then? i.e. explicitly explain that the callback must be called inside _write (sync) or in a future tick (async).

[addaleax]

@mcollina Your concern is definitely valid, but do you have other suggestions, other than just documenting the current state? I do feel like this is a bug worth fixing…

[mcollina]

I think the solution in 4d7eddc is perfectly feasible and solves the problem at a cost of an additional nextTick in case a write errors.

I'm open to other fixes, but I'm strongly -1 in adding a feature that cannot be ported to readable-stream easily.

[mscdex]

Also this solution incurs some significant performance regressions:

                                                                                confidence improvement accuracy (*)   (**)  (***)
streams/writable-manywrites.js callback='no' writev='no' sync='no' n=2000000           ***    -28.95 %       ±1.50% ±1.99% ±2.59%
streams/writable-manywrites.js callback='no' writev='no' sync='yes' n=2000000          ***    -30.11 %       ±3.86% ±5.18% ±6.82%
streams/writable-manywrites.js callback='no' writev='yes' sync='no' n=2000000          ***    -16.49 %       ±3.07% ±4.10% ±5.39%
streams/writable-manywrites.js callback='no' writev='yes' sync='yes' n=2000000         ***    -31.43 %       ±2.28% ±3.04% ±3.99%
streams/writable-manywrites.js callback='yes' writev='no' sync='no' n=2000000          ***    -28.67 %       ±1.74% ±2.31% ±3.01%
streams/writable-manywrites.js callback='yes' writev='no' sync='yes' n=2000000         ***    -32.39 %       ±2.74% ±3.65% ±4.76%
streams/writable-manywrites.js callback='yes' writev='yes' sync='no' n=2000000         ***    -16.59 %       ±3.40% ±4.56% ±6.00%
streams/writable-manywrites.js callback='yes' writev='yes' sync='yes' n=2000000        ***    -30.27 %       ±2.81% ±3.75% ±4.92%

[ronag]

Also this solution incurs some significant performance regressions:

Yes, that's expected. We should inline the sync accessor. When that is done I don't expect any significant performance impact (I hope), i.e. I think this can be fixed.

[ronag]

I think the solution in 4d7eddc is perfectly feasible and solves the problem at a cost of an additional nextTick in case a write errors.

This doesn't actually fix the root problem though. It just fixes one of the edge cases with the 'error' event. The same problem occurs (I think) with other events such as 'drain' and 'final' and possibly other less obvious edge cases.

[ronag]

This might not be exactly correct since it will only run if there is a nextTick scheduled and won't take other possible async entry points into account such as IO and what not. This is not my area of expertise.

Maybe would require some help from @addaleax if this approach is deemed worth looking further into.

[addaleax]

Yeah, I’m happy to investigate this in more detail… It looks like you’re essentially going for an id for each individual synchronous block of JS execution, which should be doable, but I agree that it makes sense to get a green light for this approach first :)

[jasnell]

btw, fwiw I've been mulling over the details of a proposal to TC-39 that does precisely that: assign execution and trigger IDs for every JS execution at the language level. It would make several things significantly easier.

I'm not convinced that this PR is the way to go forward tho and definitely need to give it a bit more thought.

[addaleax]

Hm yeah … I’m not sure if this would work fully when using async_hooks because we’d basically always want the lowest async id on the stack, not the highest. But maybe we could expose + use that? That would also solve the readable-stream issue for all supported versions of Node.js (although obviously not browsers…)

[ronag]

Maybe @lpinca has some input/ideas on this?

[ronag]

@mcollina: one way to get around the readable-stream problem could be:

process.tickId === undefined || this.tick === process.tickId

The downside would be that it would be slower for sync streams (but probably correct?) on old node versions, i.e . it would always think it is "sync" and use nextTick even though it is not strictly necessary.

We could wait with backporting this to readable-stream until v14 becomes LTS?

[mcollina]

readable-stream has to work on old version of Node.js as well, as long as browsers. I would prefer to avoid this completely.

[ronag]

I think the consensus was to document the limitation since this cannot be easily ported to readable-stream. Closing.