Uh oh!
There was an error while loading. Please reload this page.
stream: don't deadlock duplexpair - #29836
Conversation
If nothing is buffered then _read will not be called and the callback will not invoked, effectivly deadlocking. Fixes: nodejs#29758
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
If we have no readableLength _read will never be called.
There was a problem hiding this comment.
Just to be clear that I’m not misunderstanding – this is because _read() was already called?
There was a problem hiding this comment.
But wouldn’t .push() schedule another ._read() call, because the readable side’s buffer is empty once the data from .push() is used?
There was a problem hiding this comment.
@addaleaxpush(chunk) should not schedule another read if push(null) is called in the same tick.
There was a problem hiding this comment.
The current implementation disregards this invariant of Readable.
There was a problem hiding this comment.
This made me consider why push(null) is called before _read. Could be that _final is called before all writes having finished. Let me check.
There was a problem hiding this comment.
no, it simply that an empty push will not trigger another read, as it is sensible to assume that if we couldn't push data then there is no data
There was a problem hiding this comment.
I mean, that kind of makes sense, but it’s also weird that .push() with an empty chunk leads to different behaviour than .push() with a non-empty chunk…
There was a problem hiding this comment.
Yea, I've been trying to get my head around it for a few days. But as you write, it's weird but makes sense. The alternative is more confusing and leads to other problems such as livelocks.
addaleax
left a comment
There was a problem hiding this comment.
Yeah, this variant seems a bit more intuitive to me 👍
lpinca
commented
Oct 3, 2019
This is basically the workaround I suggested in #29758 (comment). |
ronag
commented
Oct 3, 2019
Yep, it took me a bit longer to come to the same conclusion. |
awwright
commented
Oct 4, 2019
To rephrase #29649 (comment): this looks good, I'm just surprised it requires a fix to the DuplexPair implementation (the part I was hoping to not change, since that code is copied in so many places)—but I guess that makes sense. |
nodejs-github-bot
commented
Oct 5, 2019
nodejs-github-bot
commented
Oct 5, 2019
If nothing is buffered then _read will not be called and the callback will not be invoked, effectivly deadlocking. Fixes: #29758 PR-URL: #29836 Refs: #29649 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Trott
commented
Oct 6, 2019
Landed in 0b495a8...28c3a9d |
If nothing is buffered then _read will not be called and the callback will not be invoked, effectivly deadlocking. Fixes: #29758 PR-URL: #29836 Refs: #29649 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
If nothing is buffered then _read will not be called and the callback will not invoked, effectivly deadlocking.
Fixes: #29758
Refs: #29649
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes