http: relax requirements on upgrade listener

doc/api/http.md

@@ -471,8 +471,9 @@ added: v0.1.94
 * `head` {Buffer}
 
 Emitted each time a server responds to a request with an upgrade. If this
-event is not being listened for, clients receiving an upgrade header will have
-their connections closed.
+event is not being listened for and the response status code is 101 Switching
+Protocols, clients receiving an upgrade header will have their connections
+closed.
 
 A client server pair demonstrating how to listen for the `'upgrade'` event.
 
@@ -873,16 +874,20 @@ per connection (in the case of HTTP Keep-Alive connections).
 ### Event: 'upgrade'
 <!-- YAML
 added: v0.1.94
+changes:
+  - version: REPLACEME
+    pr-url: REPLACEME
+    description: Not listening to this event no longer causes the socket
+                 to be destroyed if a client sends an Upgrade header.
 -->
 
 * `request` {http.IncomingMessage} Arguments for the HTTP request, as it is in
   the [`'request'`][] event
 * `socket` {net.Socket} Network socket between the server and client
 * `head` {Buffer} The first packet of the upgraded stream (may be empty)
 
-Emitted each time a client requests an HTTP upgrade. If this event is not
-listened for, then clients requesting an upgrade will have their connections
-closed.
+Emitted each time a client requests an HTTP upgrade. Listening to this event
+is optional and clients cannot insist on a protocol change.
 
 After this event is emitted, the request's socket will not have a `'data'`
 event listener, meaning it will need to be bound in order to handle data

lib/_http_client.js

@@ -426,7 +426,7 @@ function socketOnData(d) {
     req.socket._hadError = true;
     req.emit('error', ret);
   } else if (parser.incoming && parser.incoming.upgrade) {
-    // Upgrade or CONNECT
+    // Upgrade (if status code 101) or CONNECT
     var bytesParsed = ret;
     var res = parser.incoming;
     req.res = res;
@@ -453,7 +453,7 @@ function socketOnData(d) {
       req.emit(eventName, res, socket, bodyHead);
       req.emit('close');
     } else {
-      // Got Upgrade header or CONNECT method, but have no handler.
+      // Requested Upgrade or used CONNECT method, but have no handler.
       socket.destroy();
     }
   } else if (parser.incoming && parser.incoming.complete &&
@@ -492,6 +492,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
   }
   req.res = res;
 
+  // Skip body and treat as Upgrade.
+  if (res.upgrade)
+    return 2;
+
   // Responses to CONNECT request is handled as Upgrade.
   const method = req.method;
   if (method === 'CONNECT') {

lib/_http_common.js

@@ -83,6 +83,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
   parser.incoming.httpVersionMinor = versionMinor;
   parser.incoming.httpVersion = `${versionMajor}.${versionMinor}`;
   parser.incoming.url = url;
+  parser.incoming.upgrade = upgrade;
 
   var n = headers.length;
 
@@ -101,19 +102,6 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method,
     parser.incoming.statusMessage = statusMessage;
   }
 
-  if (upgrade && parser.outgoing !== null && !parser.outgoing.upgrading) {
-    // The client made non-upgrade request, and server is just advertising
-    // supported protocols.
-    //
-    // See RFC7230 Section 6.7
-    upgrade = false;
-  }
-
-  parser.incoming.upgrade = upgrade;
-
-  if (upgrade)
-    return 2;  // Skip body and treat as Upgrade.
-
   return parser.onIncoming(parser.incoming, shouldKeepAlive);
 }
 

lib/_http_outgoing.js

@@ -98,7 +98,6 @@ function OutgoingMessage() {
   this.writable = true;
 
   this._last = false;
-  this.upgrading = false;
   this.chunkedEncoding = false;
   this.shouldKeepAlive = true;
   this.useChunkedEncodingByDefault = true;
@@ -304,7 +303,6 @@ function _storeHeader(firstLine, headers) {
   // in the case of response it is: 'HTTP/1.1 200 OK\r\n'
   var state = {
     connection: false,
-    connUpgrade: false,
     contLen: false,
     te: false,
     date: false,
@@ -366,10 +364,6 @@ function _storeHeader(firstLine, headers) {
     }
   }
 
-  // Are we upgrading the connection?
-  if (state.connUpgrade && state.upgrade)
-    this.upgrading = true;
-
   // Date header
   if (this.sendDate && !state.date) {
     state.header += 'Date: ' + utcDate() + CRLF;
@@ -460,8 +454,6 @@ function matchConnValue(self, state, value) {
   while (m) {
     if (m[0].length === 5)
       sawClose = true;
-    else
-      state.connUpgrade = true;
     m = RE_CONN_VALUES.exec(value);
   }
   if (sawClose)

lib/_http_server.js

@@ -530,14 +530,14 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
     parser = null;
 
     var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade';
-    if (server.listenerCount(eventName) > 0) {
+    if (eventName === 'upgrade' || server.listenerCount(eventName) > 0) {
       debug('SERVER have listener for %s', eventName);
       var bodyHead = d.slice(bytesParsed, d.length);
 
       socket.readableFlowing = null;
       server.emit(eventName, req, socket, bodyHead);
     } else {
-      // Got upgrade header or CONNECT method, but have no handler.
+      // Got CONNECT method, but have no handler.
       socket.destroy();
     }
   }
@@ -592,6 +592,13 @@ function resOnFinish(req, res, socket, state, server) {
 function parserOnIncoming(server, socket, state, req, keepAlive) {
   resetSocketTimeout(server, socket, state);
 
+  if (req.upgrade) {
+    req.upgrade = req.method === 'CONNECT' ||
+                  server.listenerCount('upgrade') > 0;
+    if (req.upgrade)
+      return 2;
+  }
+
   state.incoming.push(req);
 
   // If the writable end isn't consuming, then stop reading

test/parallel/test-http-upgrade-advertise.js

@@ -8,7 +8,9 @@ const tests = [
   { headers: {}, expected: 'regular' },
   { headers: { upgrade: 'h2c' }, expected: 'regular' },
   { headers: { connection: 'upgrade' }, expected: 'regular' },
-  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' }
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'upgrade' },
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'destroy' },
+  { headers: { connection: 'upgrade', upgrade: 'h2c' }, expected: 'regular' },
 ];
 
 function fire() {
@@ -32,10 +34,17 @@ function fire() {
     done('regular');
   });
 
-  req.on('upgrade', function onUpgrade(res, socket) {
-    socket.destroy();
-    done('upgrade');
-  });
+  if (test.expected === 'destroy') {
+    req.on('socket', () => req.socket.on('close', () => {
+      server.removeAllListeners('upgrade');
+      done('destroy');
+    }));
+  } else {
+    req.on('upgrade', function onUpgrade(res, socket) {
+      socket.destroy();
+      done('upgrade');
+    });
+  }
 
   req.end();
 }

test/parallel/test-http-upgrade-server.js

@@ -121,8 +121,6 @@ function test_upgrade_with_listener() {
 /*-----------------------------------------------
   connection: Upgrade, no listener
 -----------------------------------------------*/
-let test_upgrade_no_listener_ended = false;
-
 function test_upgrade_no_listener() {
   const conn = net.createConnection(server.address().port);
   conn.setEncoding('utf8');
@@ -135,8 +133,9 @@ function test_upgrade_no_listener() {
              '\r\n');
   });
 
-  conn.on('end', function() {
-    test_upgrade_no_listener_ended = true;
+  conn.once('data', (data) => {
+    assert.strictEqual('string', typeof data);
+    assert.strictEqual('HTTP/1.1 200', data.substr(0, 12));
     conn.end();
   });
 
@@ -182,5 +181,4 @@ server.listen(0, function() {
 process.on('exit', function() {
   assert.strictEqual(3, requests_recv);
   assert.strictEqual(3, requests_sent);
-  assert.ok(test_upgrade_no_listener_ended);
 });