http: regression in Upgrade header handling

[bnoordhuis]

'use strict';

var assert = require('assert');
var http = require('http');
var net = require('net');

var seenUpgrade = 0;
process.on('exit', function() { assert.equal(seenUpgrade, 1); });

http.createServer(assert.fail).listen(0, '127.0.0.1', function() {
  this.on('upgrade', function(req, conn, head) {
    seenUpgrade += 1;
    conn.destroy();
    this.close();
  });
  var options = { host: this.address().address, port: this.address().port };
  net.connect(options, function() {
    this.write('GET / HTTP/1.1\r\n' +
               'Upgrade: Yes, please.\r\n' +
               '\r\n');
  });
});

Works with joyent/node@v0.11.16 but fails with iojs@5843ae8. The request callback is called and the 'upgrade' event doesn't fire. I'll bisect.

[bnoordhuis]

git-bisect has spoken:

598efcbe7f4d795622f038e0ba28c7b119927a14 is the first bad commit
commit 598efcbe7f4d795622f038e0ba28c7b119927a14
Author: Fedor Indutny <fedor@indutny.com>
Date:   Wed Jan 14 16:00:26 2015 +0300

    deps: update http_parser to 2.4.1

    PR-URL: https://github.com/iojs/io.js/pull/397
    Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

I blame the reviewer.

[bnoordhuis]

Fixed in 6605096.

[indutny]

I think that the test case needs some justification. Why should this behavior be supported, who does experience the problems with it, and why shouldn't that code be fixed instead?

cc @bnoordhuis

[indutny]

(See nodejs/http-parser#237 for compromise solution, but let's discuss it first)

[Fishrock123]

@indutny this is in master from http-parser@2.5.0 right?

[indutny]

Oh right!