-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
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.
I would add a test for this. |
Floating fix for nodejs/node#31756
Yep, was just working one up :-) |
Hm, this might actually be an even bigger problem. We rely on |
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 |
Ok, just a quick check... and yes, without this fix, everything works correctly in my test so long as the callback passed in to |
Another possible solution is to set |
@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 |
Yeah, definitely would rather prefer to avoid that. If this PR doesn't land, calling the In testing, I did notice another oddity in |
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. |
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 |
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. |
Since I'm able to work around the issue successfully I'm fine with this not landing. |
Updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you please squash your commits and force push? It seems the Github UI is not showing the actual fix to |
@mcollina See #31756 (comment) – this doesn’t currently come with a fix.
@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? |
@mcollina ... the fix to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronag ... what is it that you're objecting to with this PR now? |
Ah, sorry. All is good. |
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
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>
Landed in 5bef2cc |
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>
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>
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>
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>
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>
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