stream: Duplex autoDestroy with disabled readable/writable#32139
stream: Duplex autoDestroy with disabled readable/writable#32139ronag wants to merge 4 commits intonodejs:masterfrom
Conversation
|
This requires another @nodejs/tsc review |
|
@jasnell: This might be of interested to you in relation to the quic work. |
|
I have no specific objection but I think we should not change the behavior of a generic base class to make it work like one of its derived class. |
Though I would argue that this behavior makes sense... regardless where it's based from? |
@mcollina Do you approve this? I don't usually approve |
mcollina
left a comment
There was a problem hiding this comment.
I would need to know more about this change? Can you enrich the description a little bit? It seems very subtle.
I'm especially worried that for a semver-major change we only have so little tests. I would like to ask for a test that covers this behavior.
|
Yea, I totally missed the tests. Sorry about that. I'll sort this out. |
|
@mcollina: I have updated the description. Please take a look and let me know if it makes sense. I'll make some proper tests if you find it sensible. |
|
I'm good with this change, although with quic we cannot use autoDestroy as the lifecycle of the object is not tied only to the writable/readable state. |
This comment has been minimized.
This comment has been minimized.
|
@nodejs/streams This PR also fixes the following comment: |
mcollina
left a comment
There was a problem hiding this comment.
Can you add a few tests for the change in behavior?
stream.Duplex and net.Socket slightly differs in behavior. Especially when it comes to the case where one side never becomes readable or writable. This aligns Duplex with the behavior of Socket.
|
rebased and added tests |
2696a6e to
fdd369c
Compare
stream.Duplex and net.Socket slightly differs in behavior. Especially when it comes to the case where one side never becomes readable or writable. This aligns Duplex with the behavior of Socket. PR-URL: #32139 Reviewed-By: Anto Aravinth <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 388cef6 |
stream.Duplex and net.Socket slightly differs in behavior.
Especially when it comes to the case where one side never
becomes readable or writable. This aligns Duplex with the
behavior of Socket.
The "trick"
net.Socketdoes is to explicitly setwritable/readabletofalseinstead of callingend()/push(null)to avoid the'finish'/'end'events.This PR extract the streams part of #31806 for easier review.
---- EDIT UPDATED DESCRIPTION ----
Some streams are implemented as
Duplexbut don't know whether they are unidirectional or not until runtime. In the case that it is determined as unidirectional then one of the sides need to be disabled for e.g.autoDestroy,stream.finishedto work properly. However, it doesn't make sense to callend()/push(null)since it never werewritable/readableand emitting'finish'/'close'is weird/confusing.They way e.g.
net.Socketresolves it (and maybe quic and other implementors might want to resolve it? @jasnell) is to explicitly setwritable/readabletofalseonce/if it has determined the stream to be unidirectional. This works already, however it would breakautoDestroywhich (before this PR) waits for both sides to complete, when (in this case) only one is expected complete.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes