stream: Don't emit 'end' unless read() called

[isaacs]

This solves the problem of calling readable.pipe(writable) after the
readable stream has already emitted 'end', as often is the case when
writing simple HTTP proxies.

The spirit of streams2 is that things will work properly, even if you
don't set them up right away on the first tick.

This approach breaks down, however, because pipe()ing from an ended
readable will just do nothing. No more data will ever arrive, and the
writable will hang open forever never being ended.

However, that does not solve the case of adding a on('end') listener
after the stream has received the EOF chunk, if it was the first chunk
received (and thus, length was 0, and 'end' got emitted). So, with
this, we defer the 'end' event emission until the read() function is
called.

Also, in pipe(), if the source has emitted 'end' already, we call the
cleanup/onend function on nextTick. Piping from an already-ended stream
is thus the same as piping from a stream that is in the process of
ending.

Updates many tests that were relying on 'end' coming immediately, even
though they never read() from the req.

Fix #4942

[indutny]

I'm worried about calling 'end' listener on already ended streams... Is it really needed? Can't we just handle this in pipe?

[isaacs]

the issue is that you can do stuff like this:

server.listen(function(q, s){
  sessionCheckDb(q, function(er, ok) {
    if (ok) {
      q.on('data', blah);
      q.on('end', endIt);
    } else {
      s.end('please log in');
    }
  })
})

So, even without pipe(), it still can be an issue.

[jwchang0206]

@isaacs

This commit breaks my code :(

so I tried

git checkout cd2b9f542c34ce59d2df7820a6237f7911c702f3

which is the commit right before this one.

And the broken code has disappeared.

So I believe not sending "end" seriously breaking existing codes

[indutny]

It might break your code, but generally if your code broke because of it - you were using API in a wrong way.

Can you please post part of your code or reduced test case to demonstrate your problem?

[jwchang0206]

@indutny knox https://github.com/LearnBoost/knox is the only module which is using stream where I have trouble with. I'm looking at lib/client.js.

[isaacs]

@inspiredjw What does "break" mean? What is happening?

The two things that might be affected by this are:

1. HTTP server requests that you are listening for 'end' but never reading from.
2. HTTP client responses that you are not ever consuming (but ARE waiting for 'end' events on.)

In https://github.com/LearnBoost/knox/blob/master/lib/client.js, I see that you are listening for an 'end' event in one place. However, in that same place, you are also listening for 'data', so that will consume the stream, and should not be an issue.

There is another place which is harder to figure out, and I think is probably the issue:

function registerReqListeners(req, fn){
  req.on('response', function(res){ fn(null, res); });
  req.on('error', fn);
}

Here, you're listening to the 'response' event, so node's http implementation is not going to throw it away. It is now the responsibility of that function to consume the response. If that function does not ever do anything with the response object, then it will never emit an 'end' event. This was already an issue prior to the referenced commit, but you were probably making requests that did not have response bodies.

An actual failing test would be great here.

[jwchang0206]

https://github.com/LearnBoost/knox

make test

Knox's tests fail Automattic/knox#146 (comment)