Stream pump #13506
Stream pump #13506
Conversation
nodejs-github-bot
added
build
lib/_stream_pump.js
Outdated
| @@ -0,0 +1,191 @@ | |||
| // The MIT License (MIT) | |||
| // | |||
| // Copyright (c) 2014 Mathias Buus | |||
There was a problem hiding this comment.
Is the idea that this file keeps getting vendored in? So I guess review comments on the code here make limited sense?
There was a problem hiding this comment.
On a semi-related note, I wish we could collect any/all vendored code under a single directory to better indicate that PRs/issues/etc. should not be opened in nodejs/node for those files.
There was a problem hiding this comment.
I think we can drop the copyright, as stated by @mafintosh in mafintosh/pump#17 (comment). Are you still of that idea, @mafintosh?
test/parallel/test-stream-pump.js
Outdated
| 'use strict'; | ||
|
|
||
| const stream = require('stream'); | ||
| require('../common'); |
There was a problem hiding this comment.
This needs to be the very first require()
test/parallel/test-stream-pump.js
Outdated
| const assert = require('assert'); | ||
| const crypto = require('crypto'); | ||
| const pump = stream.pump; | ||
| // tiny node-tap lookalike. |
There was a problem hiding this comment.
I’m really not keen on introducing more of these in the tests, the ones that use “tiny node-tap lookalike” are usually far from readable…
There was a problem hiding this comment.
I'm actually 👎 on using the "tiny" node-tap lookalike. They are very hard to understand.
There was a problem hiding this comment.
this is because I copied some other test that had that in it
doc/api/stream.md
Outdated
| #### Class Method: stream.pump(...streams[, callback]) | ||
|
|
||
| * two or more streams to pipe between | ||
| * optional callback |
There was a problem hiding this comment.
These parameters should be formatted according to the style used in the rest of the documentation.
Also, it would be a good idea to either add the function signature here in some way or at the very least describe in the description below what parameters the callback should have.
|
I always use pump, agree its better than pipe. Isn't the consensus among stream experts that no one should never use I'm sure the alternative (documenting in the streams API that its docs are present to describe how streams work in the node builtins, but that user code should not require this module directly and instead use the npm-installable alternatives - with a list of them, perhaps bringing the best into github.com/nodejs/...) was considered, maybe describe in this PR why its better to just vendor in pump? Will we recommend using npm installable pump (for the benefits of semver, etc.)? |
|
If I'm reading this correctly, this adds another API to Stream, IMO, this is not a suitable solution for Core. If this is the "right way" to do this, then it should replace I don't think it's a good practice for Core to include two APIs that do essentially the same thing. If there is a consensus that this is better then lets move to it. If there is not a consensus then, as annoying as it might be, we should stick with the current behavior. If memory serves, we always wanted to figure out how to forward errors properly but ended up cutting attempts to do so in order to ship major releases on time that included large breaking upgrades to streams. It sounds like the kinks in this were worked out by this module in the ecosystem and if it doesn't break the ecosystem I think it would be a huge benefit to make an improvement to the Core |
|
@mikeal Concensus among stream WG members is that breaking backwards
compatibility is unacceptable, but also that core should at least provide a
valid answer on how to use streams.
A hard deprecate is undesirable as the fallout would be huge, tho I'm with
you if you're suggesting .pipe() should replaced with pump() in all the
doc's example code (e.g. soft deprecate).
…On Wed, Jun 7, 2017, 00:46 Mikeal Rogers ***@***.***> wrote:
If I'm reading this correctly, this *adds* another API to Stream .pump()
which does essentially what .pipe() does but "better."
IMO, this is not a suitable solution for Core.
If this is the "right way" to do this, then it should replace .pipe(). We
should build a consensus that this is the correct API and migrate as a
breaking change (error forwarding certainly qualifies as a breaking change).
I don't think it's a good practice for Core to include two APIs that do
essentially the same thing. If there is a consensus that this is better
then lets move to it. If there is not a consensus then, as annoying as it
might be, we should stick with the current behavior.
If memory serves, we always wanted to figure out how to forward errors
properly but ended up cutting attempts to do so in order to ship major
releases on time that included large breaking upgrades to streams. It
sounds like the kinks in this were worked out by this module in the
ecosystem and if it doesn't break the ecosystem I think it would be a huge
benefit to make an improvement to the Core pipe() API.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#13506 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACWleqfavwhnv-wksAV5SsPiSx0EDzPgks5sBdbCgaJpZM4Nxyrs>
.
|
|
@yoshuawuyts how breaking would the change be? Is there something beyond the error forwarding that I'm not seeing? |
|
@mikeal also that all streams close when any one of them exits. I think
there was something else, but I forgot.
The value of `pump()` is that it also provides a reliable handle when
streams exit (finish, close, destroy, end, error, etc.) - something that
requires a non-trivial amount of wiring otherwise (especially taking in
account the different Node versions and internal changes that have occurred
over time).
…On Wed, Jun 7, 2017 at 1:14 AM Mikeal Rogers ***@***.***> wrote:
@yoshuawuyts <https://github.com/yoshuawuyts> how breaking would the
change be? Is there something beyond the error forwarding that I'm not
seeing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#13506 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACWleuQFvYlJgyfYSDstEc14hM-IyYUYks5sBd1ogaJpZM4Nxyrs>
.
|
|
@yoshuawuyts That seems like an actual bug in the current implementation. I'm not arguing that a change this substantial would not break a few people, but have we actually quantified how much it would break? We have tools to help us figure this out (CITGM) and we can even stage the change in the next major's nightlies and introduce it in a version not slated for LTS: the upcoming v7. We're actually at an optimal time to try this. If the break is too large we can back it out. If we run CITGM and a bunch of packages break we can drop this entire line of argument, but before we add a redundant API out of this particular fear we should try our best to quantify the effect it would have. We're no longer completely in the dark about how much ecosystem impact changes have and we don't need to make decisions without any data the way we have traditionally had to. |
|
Since the CI is free I'm running CITGM on refack@2993ff2 Our test suite has "only" 10 failing tests: https://ci.nodejs.org/job/node-test-commit-linuxone/6435/nodes=rhel72-s390x/tapResults/ |
Well |
|
@refack that error is because pump() doesn't return the destination stream like pipe() does. You'll need to account for that in your glue code. |
|
@mafintosh ah, that makes sense, when I saw that it supported multiple streams I assumed the feature was just cut ;) |
|
refack@ccdbedd |
|
The fails on linuxone all appear to be because of the same bug and is probably fixable. I'm seeing similar failures in the smoker. Any thoughts @mafintosh function _addListener(target, type, listener, prepend) {
^
RangeError: Maximum call stack size exceeded
at _addListener (events.js:232:22)
at Extract.addListener (events.js:298:10)
at Extract.Abstract.on (/data/iojs/build/workspace/citgm-smoker/nodes/rhel72-s390x/smoker/lib/node_modules/citgm/node_modules/fstream/lib/abstract.js:18:25)
at eos (_stream_pump.js:106:10)
at destroyer (_stream_pump.js:137:3)
at streams.map (_stream_pump.js:176:12)
at Array.map (native)
at Function.pump (_stream_pump.js:173:28)
at Extract.Stream.pipe (stream.js:37:17)
at Extract.Reader.pipe (/data/iojs/build/workspace/citgm-smoker/nodes/rhel72-s390x/smoker/lib/node_modules/citgm/node_modules/fstream/lib/reader.js:235:32) |
|
@mikeal doh i'm being silly. pump calls pipe internally haha. we'll have to inline the implementation for an actual test |
|
Yep, started stack-overflowing 🤷♂️ _stream_pump.js:40
function eos(stream, opts, _callback) {
^
RangeError: Maximum call stack size exceeded
at eos (_stream_pump.js:40:13)
at destroyer (_stream_pump.js:137:3)
at streams.map (_stream_pump.js:176:12)
at Array.map (native)
at Function.pump (_stream_pump.js:173:28)
at Extract.Stream.pipe (stream.js:37:17)
at Extract.Reader.pipe (/home/iojs/build/workspace/citgm-smoker/nodes/ppcbe-ubuntu1404/smoker/lib/node_modules/citgm/node_modules/fstream/lib/reader.js:235:32)
at pipe (_stream_pump.js:161:33)
at Array.reduce (native)
at Function.pump (_stream_pump.js:188:18)
Build step 'Conditional steps (multiple)' marked build as failure
Recording test results
ERROR: Step ?Publish JUnit test result report? failed: No test report files were found. Configuration error?
Notifying upstream projects of job completion
Finished: FAILURE |
|
easiest way to test this would be to set |
No. Adds a higher-level API that does pump(streamA, streamB, streamC, function (err) {
// err can originate in any of the streams
})The hard part of forwarding errors in
I disagree, and I hope this is different enough for you to change your mind.
I think that a function that consider a pipeline of streams vs a method on the single stream is the way to go. It has been proven in production several times, and the API is easy to read.
Absolutely.
I do not see how that has changed. Back then, we wanted to move fast. Right now, we have a massive ecosystem to move forward.
If it is a bug or not it is not really the point here. As things are now, The current best practice is to use I'm perfectly fine if we decide to not have this in core, but it is a nice to have and it will simplify discovery for newbies. It would also mean that all the core components for using streams successfully will be released together.
The change as you propose it is a breaking change, so we cannot release it in a non-major release of Node. Also, it would create a lot of churn in the community, as we would have to bump semver-major on readable-stream, and then on every dependants.
I'd be very happy to test an alternative implementation that does that. Even if there are no breakages (which I truly doubt), the point still remains: I'm 👎 on changing the behavior of |
lib/_stream_pump.js
Outdated
|
|
||
| function isRequest(stream) { | ||
| return stream.setHeader && isFn(stream.abort); | ||
| } |
There was a problem hiding this comment.
we need a test that exercises http request and fs.
I think you can inline the typeof fs === 'function' check.
lib/_stream_pump.js
Outdated
| throw new Error('pump requires two streams per minimum'); | ||
|
|
||
| let error; | ||
| const destroys = streams.map((stream, i) => { |
There was a problem hiding this comment.
I would prefer not to close over this here, as it might be the global Stream object.
lib/_stream_pump.js
Outdated
| return typeof fn === 'function'; | ||
| } | ||
|
|
||
| function eos(stream, opts, _callback) { |
There was a problem hiding this comment.
we might also expose this, as it is another must-have.
| function isChildProcess(stream) { | ||
| return stream.stdio && | ||
| Array.isArray(stream.stdio) && stream.stdio.length === 3; | ||
| } |
There was a problem hiding this comment.
we need a test for child processes as well.
Yes. When/If this is released, we will pick it up in |
Describing it this way makes it sound like this is something that should be kept in the ecosystem. I think a definitive question that we should ask is: If this is merged is there any case where we would advise users to use |
My suggestion was to stick it in the next major :) |
|
@calvinmetcalf I guess it would be good if you could get around to pushing the changes you did so far :-) |
|
as long as this isn't going to land yet, can I make a request that _stream_pump be moved to internal/streams/pump |
|
Big 👍 on adding |
|
@calvinmetcalf @mcollina @mafintosh I am going to close this PR due to no further progress in the next few days. If someone wants to pick this up and update it before that, that would be great! |
|
Can you leave this open? Thanks. |
BridgeAR
force-pushed
the
stream-pump
branch
from
0ae5cba to
556e7c7
Compare
|
I just rebased and also addressed a few comments but not all. |
|
I would really like to see this landed. What needs to be done? |
|
Should we update the code to match the latest version of the pump module? |
|
Definitely. Most importantly, we need to port the unit tests too. |
|
@mcollina @targos @calvinmetcalf Do you need any help with this? Or this one is only for experienced node core devs? |
|
@prog1dev we need help, and yes, this is probably going to be an hard one! If you want to help, feel free to contribute anyway - we need all the help we can get |
lib/_stream_pump.js
Outdated
| @@ -150,14 +150,14 @@ function destroyer(stream, reading, writing, _callback) { | |||
| return stream.abort(); | |||
| // request.destroy just do .end - .abort is what we want | |||
|
|
|||
| if (isFn(stream.destroy)) return stream.destroy(); | |||
| if (isFn(stream.destroy)) return stream.destroy(err); | |||
There was a problem hiding this comment.
fyi, this wont work on older streams, which is why it was not calling it with err before
lib/_stream_pump.js
Outdated
| // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
|
@mafintosh as a collaborator you can now push changes to this branch to make sure this gets in a good state. Alternatively you can of course also open a new PR. |
|
Closing in favor of #19828 |
Docs not finished.
As discussed at the streams working group face to face in Berlin we wanted to bring @mafintosh's pump module into core. The reason being that pipe is fundamentally broken since it doesn't forward errors or clean up so the advice given is always not use it and instead use the pump module. The consensus at the meeting was that if a user land module is mandatory to use streams, it should probably be part of streams.
see nodejs/readable-stream#283, mafintosh/pump#17
cc @nodejs/streams