http: IncomingMessage not emitting 'end'

[mscdex]

• Version: v4.5.0+
• Platform: all
• Subsystem: http

@davidmarkclements brought to my attention an issue that came about in node v4.5.0 where an http.IncomingMessage's 'end' event wasn't being emitted properly when piping to a stream.Writable. The event does get emitted in node v4.4.7 however. Bisecting reveals 6f312b3 as the offending commit.

Test case:

var http = require('http');
var Writable = require('stream').Writable;

http.createServer((req, res) => {
  req.on('end', function() {
    console.log('end');
  });
  var ws = new Writable({
    write: function(data, enc, cb) {
      cb();
      this.emit('finish');
    }
  });
  req.pipe(ws);
}).listen(8080)

It doesn't matter which order the cb() and this.emit('finish') are in or if the emit() is wrapped in a process.nextTick(). If emit() is wrapped in a setImmediate() though, then the 'end' event gets emitted.

/cc @trevnorris @indutny

[indutny]

Looking.

[indutny]

Why should it happen, though? Here is a reduced example without http:

var Readable = require('stream').Readable;
var Writable = require('stream').Writable;

var ws = new Writable({
  write: function(data, enc, cb) {
    cb();
    this.emit('finish');
  }
});

var c = 0;

var rs = new Readable({
  read: function() {
    if (++c === 1) {
      this.push('ohai');
    } else {
      setImmediate(() => {
        this.push(null);
      });
    }
  }
});

rs.on('end', () => console.log('end'));
rs.pipe(ws);

rs is unpiped on finish event, and if the end is supposed to happen slightly later - it won't be unqueued because there are no data listeners.

[davidmarkclements]

cc @mcollina

[mscdex]

@indutny In the original case (the busboy module) all of the request's data has already been read when 'finish' is emitted, so I would have expected 'end' to be emitted when there is no data left?

[indutny]

@mscdex this solely depends on the timing of other side and when it is going to call this.push(null). Emitting finish manually is rather uncommon, and I'd say incorrect from the point of view of Streams API. No surprise that it causes such issues.

[mcollina]

I looked on the current streams source code, and 'finish' should not be emitted manually.
See https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L515. Both dicer and busboy have a lot of custom logic attached to 'finish', and they override emit to do some specific stuff on 'finish'. @mscdex, I'm happy to help if you want to start a refactoring of both.

Now, why that commit caused this is out of my grasp.

[indutny]

@mcollina is think that commit has slightly changed timing of nextTicks which caused this. Do you think we should close the issue at this point? Seems to be "wontfix" to me.

[mcollina]

@indutny I agree with you. But maybe there is a let's leave this open for a couple of days, maybe there is a missing angle we are not seeing.

[indutny]

No feedback, closing. Feel free to reopen this issue if there is any additional details we need to be aware of.