stream: add known_issue test for sync callback with error

[jasnell]

If the write callbacks are invoked synchronously with an
error, onwriteError would cause the error event to be
emitted synchronously, making it impossible to attach
an error handler after the call that triggered it.

Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341

[mcollina]

I would add a test for this.

[jasnell]

Yep, was just working one up :-)

[ronag]

Hm, this might actually be an even bigger problem.

We rely on state.sync which we set to false after _write has been invoked, assuming that the callback (if not already invoked) then will be invoked in the next tick. However, in this edge case that is not true. So I think the same problem will apply to other events such as 'drain' and 'finish'.

[jasnell]

That's entirely possible. In my particular edge case, there are two times where the callback will be called sync: when the data is zero length (success) or when the write is canceled. @mcollina did suggest that another way of addressing this could be to call the _destroy() callback in a nextTick, which likely would address my specific case but that would likely sidestep the potential issue here. While it's obviously an edge case, this definitely brings out some broken behavior

[jasnell]

Ok, just a quick check... and yes, without this fix, everything works correctly in my test so long as the callback passed in to _destroy() is called with process.nextTick(). However, if the a streams implementation, for any reason, decides to invoke the write callback synchronously with an error, the problem here remains.

[ronag]

Another possible solution is to set state.sync = false through a nextTick. I think that's probably the safest and could cover all the edge cases. Though that might have performance implications.

[ronag]

@mcollina's suggest also sounds viable. But then I think we should update the docs with an explicit invariant that the callback must either be invoked synchronously inside _write or in a nextTick.

[jasnell]

Yeah, definitely would rather prefer to avoid that. If this PR doesn't land, calling the _destroy() callback in a nextTick works for my immediate need, but this one is still a bit concerning.

In testing, I did notice another oddity in onwriteError() ... As expected, any buffered write requests are canceled via errorBuffer(), however, the callbacks are always called with the ERR_STREAM_DESTROYED error which has the error message, 'Cannot call write after a stream was destroyed'. This is misleading because the writes were called before the stream was destroyed, they were just canceled. It would likely be better, albeit semver-major, to use a different error message when canceling buffered writes due to an error.

[jasnell]

@mcollina's suggest also sounds viable. But then I think we should update the docs with an explicit invariant that the callback must either be invoked synchronously inside _write or in a nextTick.

I don't think that's a long term fix but we definitely should document it until we can get around to identifying the full range of issues on this.

[ronag]

A more performant solution could use something like a "tick counter" i.e a counter that counts up (and overflows) with every tick. Then we could replace the state.sync with that and compare the tick number instead. That could resolve this without a performance impact. Though I don't think that feature exists at the moment...

[ronag]

For the record I'm -1 on this PR. It will just hide the most obvious problem but there might be other related problems as well. Making sure the callback is invoked in a nextTick in user land would be preferable until a better solution is found.

[jasnell]

Since I'm able to work around the issue successfully I'm fine with this not landing. I'll close and open an issue instead. Actually, rather than closing, I'll make the test a known-issue test and back out the actual change to _stream_writable

[jasnell]

Updated!

[mcollina]

lgtm

[mcollina]

Can you please squash your commits and force push? It seems the Github UI is not showing the actual fix to _stream_writable.

[addaleax]

@mcollina See #31756 (comment) – this doesn’t currently come with a fix.

A more performant solution could use something like a "tick counter" i.e a counter that counts up (and overflows) with every tick.

@ronag What does “tick” refer to here? :) Do you want a counter for each event loop turn? For each call into JS from the event loop?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29095/

[ronag]

@ronag What does “tick” refer to here? :) Do you want a counter for each event loop turn? For each call into JS from the event loop?

See, #31765

[jasnell]

Can you please squash your commits and force push? It seems the Github UI is not showing the actual fix to _stream_writable.

@mcollina ... the fix to _stream_writable was backed out of this PR for the time being. This PR only adds a known_issue test for now. See the comments #31756 (comment) and #31756 (comment)

[ronag]

#31756 (comment)

[jasnell]

@ronag ... what is it that you're objecting to with this PR now?

[ronag]

Ah, sorry. All is good.

[jasnell]

Landed in 5bef2cc