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

fix: should not destroy streams #612

Closed
wants to merge 1 commit into
from

Conversation

8 participants
@dead-horse
Member

dead-horse commented Dec 2, 2015

use black-hole-stream to make sure stream's data has been read.

since user may set this.body to a http IncomingMessage, if we destroy this stream, it will destroy the http socket that make keep-alive failed, and may affect other http requests reusing this socket.
I think maybe we shouldn't destroy streams, only need to insure streams' data will be read and don't leak.

@dead-horse dead-horse added bug 1.x labels Dec 2, 2015

Show outdated Hide outdated package.json
},
"engines": {
"node": ">= 0.12.0",
"iojs": ">= 1.0.0"
"node": ">= 0.12.0"

This comment has been minimized.

This comment has been minimized.

@dead-horse

dead-horse Dec 2, 2015

Member

can we drop the support of node < 4 in koa@1 ? /cc @tj @jonathanong

@dead-horse

dead-horse Dec 2, 2015

Member

can we drop the support of node < 4 in koa@1 ? /cc @tj @jonathanong

This comment has been minimized.

@JacksonTian

JacksonTian Dec 2, 2015

oh, master is v1.

@JacksonTian

JacksonTian Dec 2, 2015

oh, master is v1.

This comment has been minimized.

@juliangruber

juliangruber Dec 4, 2015

Contributor

this should be its own pull request

@juliangruber

juliangruber Dec 4, 2015

Contributor

this should be its own pull request

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@fengmk2

This comment has been minimized.

Show comment
Hide comment
@fengmk2

fengmk2 Dec 4, 2015

Member

LGTM

Member

fengmk2 commented Dec 4, 2015

LGTM

Show outdated Hide outdated .travis.yml
- "4"
- "5"

This comment has been minimized.

@juliangruber

juliangruber Dec 4, 2015

Contributor

this should be its own pull request

@juliangruber

juliangruber Dec 4, 2015

Contributor

this should be its own pull request

Show outdated Hide outdated test/application.js
@@ -872,6 +876,69 @@ describe('app.respond', function(){
.get('/')
.expect(404, done);
})
it('should insure stream do not leak', function(done){

This comment has been minimized.

@juliangruber

juliangruber Dec 4, 2015

Contributor

insure => ensure

@juliangruber

juliangruber Dec 4, 2015

Contributor

insure => ensure

@dead-horse

This comment has been minimized.

Show comment
Hide comment
Member

dead-horse commented Dec 4, 2015

@juliangruber all fixed

Show outdated Hide outdated test/application.js
var app = koa();
let stream1 = fs.createReadStream(__filename);
let stream2 = fs.createReadStream(__filename);
stream1.once('end', done);

This comment has been minimized.

@juliangruber

juliangruber Dec 4, 2015

Contributor

i don't understand why this one would end as well? it's never read from

@juliangruber

juliangruber Dec 4, 2015

Contributor

i don't understand why this one would end as well? it's never read from

This comment has been minimized.

@dead-horse

dead-horse Dec 4, 2015

Member

we'll destroy(consume) all stream's that set to this.body here https://github.com/koajs/koa/pull/612/files#diff-d86a59ede7d999db4b7bc43cb25a1c11R166

@dead-horse

dead-horse Dec 4, 2015

Member

we'll destroy(consume) all stream's that set to this.body here https://github.com/koajs/koa/pull/612/files#diff-d86a59ede7d999db4b7bc43cb25a1c11R166

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 4, 2015

Contributor

hmmm, this seems more like an edge case to me. are there other cases that would benefit from this change?

Contributor

juliangruber commented Dec 4, 2015

hmmm, this seems more like an edge case to me. are there other cases that would benefit from this change?

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 4, 2015

Member

The most serious problem now is http keepalive sockets will be destroyed, and we must fix this issue. Not sure any other side effect if we destroy the streams.

Member

dead-horse commented Dec 4, 2015

The most serious problem now is http keepalive sockets will be destroyed, and we must fix this issue. Not sure any other side effect if we destroy the streams.

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 4, 2015

Contributor

are you sure destroying a http stream will affect other requests on the same socket? sounds more like a node bug to me

Contributor

juliangruber commented Dec 4, 2015

are you sure destroying a http stream will affect other requests on the same socket? sounds more like a node bug to me

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 4, 2015

Member

If you destroy an IncommingMessage, it will destroy the socket behind: https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L92

That will affect other IncommingMessage's that reusing the same socket and will cause socket hang up error. This is reported in ali-sdk/ali-oss#40 (sorry this issue is in chinese).

Member

dead-horse commented Dec 4, 2015

If you destroy an IncommingMessage, it will destroy the socket behind: https://github.com/nodejs/node/blob/master/lib/_http_incoming.js#L92

That will affect other IncommingMessage's that reusing the same socket and will cause socket hang up error. This is reported in ali-sdk/ali-oss#40 (sorry this issue is in chinese).

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 5, 2015

Member

any suggestions? I'll merge this PR if no response in the next day. :)

Member

dead-horse commented Dec 5, 2015

any suggestions? I'll merge this PR if no response in the next day. :)

@dead-horse dead-horse self-assigned this Dec 5, 2015

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 5, 2015

Contributor

Hey man, no need to put a deadline on it. If you need it urgently use a fork or find a userland solution.

Contributor

juliangruber commented Dec 5, 2015

Hey man, no need to put a deadline on it. If you need it urgently use a fork or find a userland solution.

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 6, 2015

Member

I think this is just a bug fix. so if anyone doubt this patch, I'll leave it here and fix in our own framework for now. :)

Member

dead-horse commented Dec 6, 2015

I think this is just a bug fix. so if anyone doubt this patch, I'll leave it here and fix in our own framework for now. :)

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 6, 2015

Contributor

this patch fails if the body stream is endless, like for example:

this.body = fs.createReadStream('/dev/urandom');

or more real-worldy

this.body = serverSentEvents(createUpdateStream());

in the cases above it's required to destroy the stream.

Contributor

juliangruber commented Dec 6, 2015

this patch fails if the body stream is endless, like for example:

this.body = fs.createReadStream('/dev/urandom');

or more real-worldy

this.body = serverSentEvents(createUpdateStream());

in the cases above it's required to destroy the stream.

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 6, 2015

Contributor

and trying to read it all out will never finish

Contributor

juliangruber commented Dec 6, 2015

and trying to read it all out will never finish

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 6, 2015

Member

although i don't think pipe an endless stream to response meaningful. but maybe there is a solution has less impact. detect the stream in destroy and don't destroy IncommingMessage(a breaking change in destroy).

Member

dead-horse commented Dec 6, 2015

although i don't think pipe an endless stream to response meaningful. but maybe there is a solution has less impact. detect the stream in destroy and don't destroy IncommingMessage(a breaking change in destroy).

fix: should not destroy streams
use black-hole-stream to make sure stream's data has been read
@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 9, 2015

Member

@juliangruber how about this?

Member

dead-horse commented Dec 9, 2015

@juliangruber how about this?

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 9, 2015

Contributor

too edge-casy. koa should not need to worry about this.

Just to be sure, this would work for you, right?

httpStream.destroy = blackHole.bind(null, httpStream);
this.body = httpStream;
Contributor

juliangruber commented Dec 9, 2015

too edge-casy. koa should not need to worry about this.

Just to be sure, this would work for you, right?

httpStream.destroy = blackHole.bind(null, httpStream);
this.body = httpStream;
@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Dec 10, 2015

Member

it is a common usage that use koa as a http proxy, and anybody who use this.body = yield request('http://foo.bar/'); //keep-alive will hit this "edge case", and it is really hard to figure out why. so I think we should fix it in framework level, because it is a bug in koa, and we can't let everyone to use httpStream.destroy = blackHole.bind(null, httpStream); hack.

Member

dead-horse commented Dec 10, 2015

it is a common usage that use koa as a http proxy, and anybody who use this.body = yield request('http://foo.bar/'); //keep-alive will hit this "edge case", and it is really hard to figure out why. so I think we should fix it in framework level, because it is a bug in koa, and we can't let everyone to use httpStream.destroy = blackHole.bind(null, httpStream); hack.

@jonathanong

This comment has been minimized.

Show comment
Hide comment
@jonathanong

jonathanong Dec 21, 2015

Member

maybe a better solution is to just wrap the http stream in another stream so you never set the http stream as the actual .body=.

const PassThrough = require('stream').PassThrough

app.use(function * (next) {
  this.body = httpStream.on('error', this.onerror).pipe(PassThrough())
})

would need docs on this though... super edge case

Member

jonathanong commented Dec 21, 2015

maybe a better solution is to just wrap the http stream in another stream so you never set the http stream as the actual .body=.

const PassThrough = require('stream').PassThrough

app.use(function * (next) {
  this.body = httpStream.on('error', this.onerror).pipe(PassThrough())
})

would need docs on this though... super edge case

@juliangruber

This comment has been minimized.

Show comment
Hide comment
@juliangruber

juliangruber Dec 22, 2015

Contributor

would need docs on this though... super edge case

what about documenting that a body stream can be .destroyed and showing how to guard against that

Contributor

juliangruber commented Dec 22, 2015

would need docs on this though... super edge case

what about documenting that a body stream can be .destroyed and showing how to guard against that

@tejasmanohar

This comment has been minimized.

Show comment
Hide comment
@tejasmanohar

tejasmanohar Jan 18, 2016

Member

what about documenting that a body stream can be .destroyed and showing how to guard against that
Show all checks

+1 @juliangruber

Member

tejasmanohar commented Jan 18, 2016

what about documenting that a body stream can be .destroyed and showing how to guard against that
Show all checks

+1 @juliangruber

@tejasmanohar tejasmanohar referenced this pull request Feb 25, 2016

Closed

Koa 2.0.0 #533

onFinish(this.res, function(){
// don't destroy http IncomingMessage, keep `keep-alive` conncetion alive.
if (val instanceof http.IncomingMessage) {
if (val.readable) val.pipe(new BlackHoleStream());

This comment has been minimized.

@jonathanong

jonathanong Feb 25, 2016

Member

why do you need black hole stream? just val.resume() to dump it

@jonathanong

jonathanong Feb 25, 2016

Member

why do you need black hole stream? just val.resume() to dump it

@jonathanong

This comment has been minimized.

Show comment
Hide comment
@jonathanong

jonathanong Feb 25, 2016

Member

anyways prefer going the documentation route. will close this when i or someone else adds documentation

Member

jonathanong commented Feb 25, 2016

anyways prefer going the documentation route. will close this when i or someone else adds documentation

@dead-horse

This comment has been minimized.

Show comment
Hide comment
@dead-horse

dead-horse Feb 26, 2016

Member

It's ok to add some warning in documentation.

Member

dead-horse commented Feb 26, 2016

It's ok to add some warning in documentation.

@dead-horse dead-horse closed this Feb 26, 2016

@dead-horse dead-horse deleted the fix-destroy-everty-stream branch Feb 26, 2016

@dead-horse dead-horse added documentation and removed bug labels Feb 26, 2016

jonathanong added a commit that referenced this pull request Mar 12, 2016

@catamphetamine

This comment has been minimized.

Show comment
Hide comment
@catamphetamine

catamphetamine Mar 6, 2017

Didn't understand a thing from the documentation on what's happening here.
It's just like "warning, never set a stream as ctx.body".

Didn't understand a thing from the documentation on what's happening here.
It's just like "warning, never set a stream as ctx.body".

@jiajianrong

This comment has been minimized.

Show comment
Hide comment
@jiajianrong

jiajianrong Aug 1, 2018

@catamphetamine, they were warning not to set a http-request-alike-stream which has keep-alive enabled. Because the framework will close the stream by default, which will break keep-alive feature.
Keep-alive means to reuse tcp socket connection for two or more http connections.

Generally, it's OK with ctx.body = fs-read-stream.
Just be careful with ctx.body = request('host:port/path').

@catamphetamine, they were warning not to set a http-request-alike-stream which has keep-alive enabled. Because the framework will close the stream by default, which will break keep-alive feature.
Keep-alive means to reuse tcp socket connection for two or more http connections.

Generally, it's OK with ctx.body = fs-read-stream.
Just be careful with ctx.body = request('host:port/path').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment