stream: Fixes missing 'unpipe' event for pipes made with {end: false}#11876
stream: Fixes missing 'unpipe' event for pipes made with {end: false}#11876zaide-chris wants to merge 2 commits intonodejs:masterfrom
Conversation
|
I was unsure if I should mark |
mcollina
left a comment
There was a problem hiding this comment.
Can you please wrap all the single tests into braces? Something like: https://github.com/nodejs/node/blob/master/test/parallel/test-buffer-badhex.js
There was a problem hiding this comment.
You could also use common.mustNotCall('unpipe should not have been emitted') in these cases.
8a07770 to
23934e4
Compare
There was a problem hiding this comment.
this is the only test that fails without this pr
|
This PR needs some more tests related to the actual use case for this to be fixed. What is the actual bug that it is causing to the user? |
|
@MylesBorins I would like to run CITGM with the special |
Would you prefer a less abstract test like: https://gist.github.com/zaide-chris/7051b80cc3f34fd0e21be639e9252f85
src.pipe(dest) and src.pipe(dest, {end: false}) act differently in more ways then what is listed in the docs so the tests pushed are based on what I wrote to find out exactly how different they are. I could not find any tests that tested for unpipe events in the case of a source stream ending even with a normal src.pipe(dest) that being the 1st test, with the one that currently fails using src.pipe(dest, {end: false}) being the 4th. So I guess the other 4 could be extraneous other then making sure both src.pipe(dest) and src.pipe(dest, {end: false}) act the same when it comes to emitting unpipe events. |
@mcollina you can just type |
|
@gibfahn this is what I'm getting: |
|
@mcollina that's odd, doing that works fine locally. I'll have a look. |
|
@mcollina for some reason the export command doesn't work through Jenkins, probably because of the way it gets quoted. I think the only other way to add the variable would be to modify the job before and after running CI. I can do that for you if you'd like. |
|
@gibfahn can we get a separate jobs on Jenkins to run this check? |
|
@mcollina adding extra jobs isn't normally a good idea if we can avoid it, otherwise the different jobs tend to get out of sync. I've added a checkbox to the job that will disable readable stream if checked (defaults to doing nothing). Don't actually have time to check it right now, but try it and let me know if it's broken. |
|
Thanks @gibfahn CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/668/ |
|
This is LGTM from me, and it seems it might take some more time to get this tested with CITGM properly, so we might get going. |
There was a problem hiding this comment.
Since this PR was opened, there were some changes to common. We have common.noop now for this type of callback. common.mustCall() can be called with no function, and it will default to common.noop.
TL;DR - common.mustCall(() => {}) throughout this PR can be replaced with common.mustCall().
No, maybe something more easy to ready, just showing that |
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: nodejs#11837
23934e4 to
a773504
Compare
|
Landed in 9dcf18a |
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: nodejs#11837
PR-URL: nodejs#11876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
This is landing but throws an error in v7.x. It depends on #12027 being backported |
|
@evanlucas would you prefer me to backport this manually? #12027 seems quite heavy. We should also backport this to v6-lts IMHO. |
|
@mcollina that would be great! You can target the |
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: nodejs#11837
PR-URL: nodejs#11876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: nodejs#11837
PR-URL: nodejs#11876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
@evanlucas here it is #12783. |
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: #11837
PR-URL: #11876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: #11837
PR-URL: #11876
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Marking as dont-land because we're landing the v7.x backport. |
|
added |
Currently when the destination emits an 'error', 'finish' or 'close'
event the pipe calls unpipe to emit 'unpipe' and trigger the clean up
of all it's listeners.
When the source emits an 'end' event without {end: false} it calls
end() on the destination leading it to emit a 'close', this will again
lead to the pipe calling unpipe. However the source emitting an 'end'
event along side {end: false} is the only time the cleanup gets ran
directly without unpipe being called. This fixes that so the 'unpipe'
event does get emitted and cleanup in turn gets ran by that event.
Fixes: #11837
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream