Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What about closing streams in the pipe on error? #22

Open
thomas-riccardi opened this issue Dec 1, 2015 · 4 comments
Open

What about closing streams in the pipe on error? #22

thomas-riccardi opened this issue Dec 1, 2015 · 4 comments

Comments

@thomas-riccardi
Copy link

https://www.npmjs.com/package/pump has similar features to multipipe, but also handles closing streams in the pipe on error.
It seems to be a nice feature for multipipe.

The issue being that destroy is not part of the Stream2/3 official API, but it's still useful for streams that support this.

Are there plans to add this to multipipe? pump has less tests than multipipe, and 2 times less downloads per months on npmjs.org

@juliangruber
Copy link
Owner

streams are already being closed, it's just that .destroy isn't being called. is that what you refer to?

@thomas-riccardi
Copy link
Author

From the pump readme:

When using standard source.pipe(dest) source will not be destroyed if dest emits close or an error. `

The flow stops, but the streams are still alive. It leaks memory, and sometimes more:

fs.createReadStream('/a/file').pipe(res) in a http server request handler: if res.socket is abruptly closed by the http client, then the fs ReadStream will not be closed because pipe doesn't do that per its spec.
This means the server is leaking file descriptors.
Cf nodejs/node#1834

pump helps by assuming destroy is pseudo-standard (and some other hacks like recognizing node-request objects which have another method for destroy (we as the community should try to converge to destroy, and add it to nodejs spec...))

@juliangruber
Copy link
Owner

ok i agree this is a good idea to add. can you contribute some of your time to get this rolling?

@thomas-riccardi
Copy link
Author

Sorry I don't have time to do that for now...
In the meantime I use pump.

Maybe both could merge!

This new feature could be considered a breaking feature, this would require major version 2 if enabled by default (and it should be enabled by default: it's really useful).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants