Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: add known_issue test for sync callback with error #31756

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 12, 2020

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

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.
@jasnell jasnell added the stream Issues and PRs related to the stream subsystem. label Feb 12, 2020
@mcollina
Copy link
Member

I would add a test for this.

jasnell added a commit to jasnell/quic that referenced this pull request Feb 12, 2020
@jasnell
Copy link
Member Author

jasnell commented Feb 12, 2020

Yep, was just working one up :-)

@ronag
Copy link
Member

ronag commented Feb 12, 2020

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
Copy link
Member Author

jasnell commented Feb 12, 2020

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
Copy link
Member Author

jasnell commented Feb 12, 2020

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
Copy link
Member

ronag commented Feb 12, 2020

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
Copy link
Member

ronag commented Feb 12, 2020

@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
Copy link
Member Author

jasnell commented Feb 12, 2020

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
Copy link
Member Author

jasnell commented Feb 12, 2020

@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
Copy link
Member

ronag commented Feb 12, 2020

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
Copy link
Member

ronag commented Feb 12, 2020

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
Copy link
Member Author

jasnell commented Feb 12, 2020

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
Copy link
Member Author

jasnell commented Feb 12, 2020

Updated!

@jasnell jasnell changed the title stream: fix onwriteError sync stream: add known_issue test for sync callback with error Feb 12, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

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

@ronag ronag mentioned this pull request Feb 13, 2020
4 tasks
@addaleax
Copy link
Member

@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?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Feb 13, 2020

@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
Copy link
Member Author

jasnell commented Feb 13, 2020

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)

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell
Copy link
Member Author

jasnell commented Feb 13, 2020

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

@ronag
Copy link
Member

ronag commented Feb 14, 2020

Ah, sorry. All is good.

ronag added a commit to nxtedition/node that referenced this pull request Feb 15, 2020
Clarifies a userland invariant until a better
solution can be found.

Also moves a misplaced sentence from _write to
write.

Refs: nodejs#31756
Refs: nodejs#31765
jasnell added a commit that referenced this pull request Feb 17, 2020
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.

PR-URL: #31756
Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Feb 17, 2020

Landed in 5bef2cc

@jasnell jasnell closed this Feb 17, 2020
ronag added a commit that referenced this pull request Feb 18, 2020
Clarifies a userland invariant until a better
solution can be found.

Also moves a misplaced sentence from _write to
write.

Refs: #31756
Refs: #31765

PR-URL: #31812
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 27, 2020
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.

PR-URL: #31756
Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
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.

PR-URL: #31756
Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
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.

PR-URL: #31756
Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
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.

PR-URL: #31756
Refs: nodejs/quic@b0d469c
Refs: nodejs/quic#341
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants