streams: pipeline error if any stream is destroyed #36791
Conversation
| pipeline([r, t, w], common.mustCall((err) => { | ||
| assert.strictEqual(err.code, 'ERR_STREAM_DESTROYED'); | ||
| assert.strictEqual(err.message, | ||
| 'Cannot call write after a stream was destroyed'); |
There was a problem hiding this comment.
Can you make the error message more helpful? E.g.: Cannot call write after a 2 stream in pipeline() was destroyed.
There was a problem hiding this comment.
hmm, it doesn't make sense since ERR_STREAM_DESTROYED will cause 2 stream to be destroyed because destination stream is passed in destroyed state -- opposite logic here. Also I am not sure if pipeline really need to be mentioned in error message.
There was a problem hiding this comment.
First destroyed stream in passed arguments should cause all previous streams in pipe chain to be destroyed
There was a problem hiding this comment.
Why only previous streams? All streams should be destroyed like what will happen when some stream is destroyed during pipeline processing.
kalenikalexander
force-pushed
the
pipeline-error-if-any-stream-is-destroyed
branch
from
d6ed804 to
bd4d601
Compare
|
@nodejs/streams |
github-actions
bot
removed
the
request-ci
|
I'm sorry this slipped through my notification stream. @ronag wdyt? |
|
Can you please rebase on top of master? Apparently something went wrong with the github merge I did. |
| } | ||
|
|
||
| if (isStream(ret) && ret.destroyed) { | ||
| finish(new ERR_STREAM_DESTROYED(reading ? 'pipe' : 'write')); |
There was a problem hiding this comment.
Why not ERR_STREAM_PREMATURE_CLOSE so it would be consistent with what will happen when the stream is destroyed during pipeline?
ronag
added
author ready
| } | ||
|
|
||
| if (isStream(ret) && ret.destroyed) { | ||
| finish(new ERR_STREAM_DESTROYED(reading ? 'pipe' : 'write')); |
There was a problem hiding this comment.
Also would be good if used the isDestroyed stream utility function.
|
@kalenikalexander You still interested in finishing this one? Sorry it slipped my notifications. |
github-actions
bot
removed
the
request-ci
targos
removed
the
author ready
8 participants
Fixes: #36674
Refs: #29227 (comment)
make -j4test (UNIX), orvcbuildtest (Windows) passes