http2: cleanup of h2 compat layer, add tests

Remove unnecessary variable assignments, remove unreachable code
pathways, remove path getter and setter, and other very minor
cleanup. Only remove reference to stream on nextTick so that
users and libraries can access it in the finish event.

Fixes: https://github.com/nodejs/node/issues/15313
Refs: https://github.com/expressjs/express/pull/3390
PR-URL: https://github.com/nodejs/node/pull/15254
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/internal/http2/compat.js

@@ -17,6 +17,21 @@ const kRawHeaders = Symbol('rawHeaders');
 const kTrailers = Symbol('trailers');
 const kRawTrailers = Symbol('rawTrailers');
 
+const {
+  NGHTTP2_NO_ERROR,
+
+  HTTP2_HEADER_AUTHORITY,
+  HTTP2_HEADER_METHOD,
+  HTTP2_HEADER_PATH,
+  HTTP2_HEADER_SCHEME,
+  HTTP2_HEADER_STATUS,
+
+  HTTP_STATUS_CONTINUE,
+  HTTP_STATUS_EXPECTATION_FAILED,
+  HTTP_STATUS_METHOD_NOT_ALLOWED,
+  HTTP_STATUS_OK
+} = constants;
+
 let statusMessageWarned = false;
 
 // Defines and implements an API compatibility layer on top of the core
@@ -24,6 +39,8 @@ let statusMessageWarned = false;
 // close as possible to the current require('http') API
 
 function assertValidHeader(name, value) {
+  if (name === '')
+    throw new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
   if (isPseudoHeader(name))
     throw new errors.Error('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED');
   if (value === undefined || value === null)
@@ -32,15 +49,11 @@ function assertValidHeader(name, value) {
 
 function isPseudoHeader(name) {
   switch (name) {
-    case ':status':
+    case HTTP2_HEADER_STATUS:    // :status
-      return true;
+    case HTTP2_HEADER_METHOD:    // :method
-    case ':method':
+    case HTTP2_HEADER_PATH:      // :path
-      return true;
+    case HTTP2_HEADER_AUTHORITY: // :authority
-    case ':path':
+    case HTTP2_HEADER_SCHEME:    // :scheme
-      return true;
-    case ':authority':
-      return true;
-    case ':scheme':
       return true;
     default:
       return false;
@@ -58,8 +71,7 @@ function statusMessageWarn() {
 }
 
 function onStreamData(chunk) {
-  const request = this[kRequest];
+  if (!this[kRequest].push(chunk))
-  if (!request.push(chunk))
     this.pause();
 }
 
@@ -71,8 +83,7 @@ function onStreamTrailers(trailers, flags, rawTrailers) {
 
 function onStreamEnd() {
   // Cause the request stream to end as well.
-  const request = this[kRequest];
+  this[kRequest].push(null);
-  request.push(null);
 }
 
 function onStreamError(error) {
@@ -86,13 +97,11 @@ function onStreamError(error) {
 }
 
 function onRequestPause() {
-  const stream = this[kStream];
+  this[kStream].pause();
-  stream.pause();
 }
 
 function onRequestResume() {
-  const stream = this[kStream];
+  this[kStream].resume();
-  stream.resume();
 }
 
 function onRequestDrain() {
@@ -101,13 +110,11 @@ function onRequestDrain() {
 }
 
 function onStreamResponseDrain() {
-  const response = this[kResponse];
+  this[kResponse].emit('drain');
-  response.emit('drain');
 }
 
 function onStreamClosedRequest() {
-  const req = this[kRequest];
+  this[kRequest].push(null);
-  req.push(null);
 }
 
 function onStreamClosedResponse() {
@@ -127,9 +134,8 @@ class Http2ServerRequest extends Readable {
   constructor(stream, headers, options, rawHeaders) {
     super(options);
     this[kState] = {
-      statusCode: null,
       closed: false,
-      closedCode: constants.NGHTTP2_NO_ERROR
+      closedCode: NGHTTP2_NO_ERROR
     };
     this[kHeaders] = headers;
     this[kRawHeaders] = rawHeaders;
@@ -155,23 +161,17 @@ class Http2ServerRequest extends Readable {
   }
 
   get closed() {
-    const state = this[kState];
+    return this[kState].closed;
-    return Boolean(state.closed);
   }
 
   get code() {
-    const state = this[kState];
+    return this[kState].closedCode;
-    return Number(state.closedCode);
   }
 
   get stream() {
     return this[kStream];
   }
 
-  get statusCode() {
-    return this[kState].statusCode;
-  }
-
   get headers() {
     return this[kHeaders];
   }
@@ -201,7 +201,10 @@ class Http2ServerRequest extends Readable {
   }
 
   get socket() {
-    return this.stream.session.socket;
+    const stream = this[kStream];
+    if (stream === undefined)
+      return;
+    return stream.session.socket;
   }
 
   get connection() {
@@ -210,7 +213,7 @@ class Http2ServerRequest extends Readable {
 
   _read(nread) {
     const stream = this[kStream];
-    if (stream) {
+    if (stream !== undefined) {
       stream.resume();
     } else {
       this.emit('error', new errors.Error('ERR_HTTP2_STREAM_CLOSED'));
@@ -218,46 +221,23 @@ class Http2ServerRequest extends Readable {
   }
 
   get method() {
-    const headers = this[kHeaders];
+    return this[kHeaders][HTTP2_HEADER_METHOD];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_METHOD];
   }
 
   get authority() {
-    const headers = this[kHeaders];
+    return this[kHeaders][HTTP2_HEADER_AUTHORITY];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_AUTHORITY];
   }
 
   get scheme() {
-    const headers = this[kHeaders];
+    return this[kHeaders][HTTP2_HEADER_SCHEME];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_SCHEME];
   }
 
   get url() {
-    return this.path;
+    return this[kHeaders][HTTP2_HEADER_PATH];
   }
 
   set url(url) {
-    this.path = url;
+    this[kHeaders][HTTP2_HEADER_PATH] = url;
-  }
-
-  get path() {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_PATH];
-  }
-
-  set path(path) {
-    let headers = this[kHeaders];
-    if (headers === undefined)
-      headers = this[kHeaders] = Object.create(null);
-    headers[constants.HTTP2_HEADER_PATH] = path;
   }
 
   setTimeout(msecs, callback) {
@@ -271,10 +251,10 @@ class Http2ServerRequest extends Readable {
     if (state.closed)
       return;
     if (code !== undefined)
-      state.closedCode = code;
+      state.closedCode = Number(code);
     state.closed = true;
     this.push(null);
-    this[kStream] = undefined;
+    process.nextTick(() => (this[kStream] = undefined));
   }
 }
 
@@ -283,12 +263,12 @@ class Http2ServerResponse extends Stream {
     super(options);
     this[kState] = {
       sendDate: true,
-      statusCode: constants.HTTP_STATUS_OK,
+      statusCode: HTTP_STATUS_OK,
-      headerCount: 0,
-      trailerCount: 0,
       closed: false,
-      closedCode: constants.NGHTTP2_NO_ERROR
+      closedCode: NGHTTP2_NO_ERROR
     };
+    this[kHeaders] = Object.create(null);
+    this[kTrailers] = Object.create(null);
     this[kStream] = stream;
     stream[kResponse] = this;
     this.writable = true;
@@ -305,13 +285,11 @@ class Http2ServerResponse extends Stream {
   }
 
   get closed() {
-    const state = this[kState];
+    return this[kState].closed;
-    return Boolean(state.closed);
   }
 
   get code() {
-    const state = this[kState];
+    return this[kState].closedCode;
-    return Number(state.closedCode);
   }
 
   get stream() {
@@ -324,7 +302,7 @@ class Http2ServerResponse extends Stream {
   }
 
   get sendDate() {
-    return Boolean(this[kState].sendDate);
+    return this[kState].sendDate;
   }
 
   set sendDate(bool) {
@@ -336,70 +314,71 @@ class Http2ServerResponse extends Stream {
   }
 
   set statusCode(code) {
-    const state = this[kState];
     code |= 0;
     if (code >= 100 && code < 200)
       throw new errors.RangeError('ERR_HTTP2_INFO_STATUS_NOT_ALLOWED');
-    if (code < 200 || code > 599)
+    if (code < 100 || code > 599)
       throw new errors.RangeError('ERR_HTTP2_STATUS_INVALID', code);
-    state.statusCode = code;
+    this[kState].statusCode = code;
+  }
+
+  setTrailer(name, value) {
+    if (typeof name !== 'string')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
+
+    name = name.trim().toLowerCase();
+    assertValidHeader(name, value);
+    this[kTrailers][name] = String(value);
   }
 
   addTrailers(headers) {
-    let trailers = this[kTrailers];
     const keys = Object.keys(headers);
-    if (keys.length === 0)
+    let key = '';
-      return;
-    if (trailers === undefined)
-      trailers = this[kTrailers] = Object.create(null);
     for (var i = 0; i < keys.length; i++) {
-      trailers[keys[i]] = String(headers[keys[i]]);
+      key = keys[i];
+      this.setTrailer(key, headers[key]);
     }
   }
 
   getHeader(name) {
-    const headers = this[kHeaders];
+    if (typeof name !== 'string')
-    if (headers === undefined)
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
-      return;
+
-    name = String(name).trim().toLowerCase();
+    name = name.trim().toLowerCase();
-    return headers[name];
+    return this[kHeaders][name];
   }
 
   getHeaderNames() {
-    const headers = this[kHeaders];
+    return Object.keys(this[kHeaders]);
-    if (headers === undefined)
-      return [];
-    return Object.keys(headers);
   }
 
   getHeaders() {
-    const headers = this[kHeaders];
+    return Object.assign({}, this[kHeaders]);
-    return Object.assign({}, headers);
   }
 
   hasHeader(name) {
-    const headers = this[kHeaders];
+    if (typeof name !== 'string')
-    if (headers === undefined)
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
-      return false;
+
-    name = String(name).trim().toLowerCase();
+    name = name.trim().toLowerCase();
-    return Object.prototype.hasOwnProperty.call(headers, name);
+    return Object.prototype.hasOwnProperty.call(this[kHeaders], name);
   }
 
   removeHeader(name) {
-    const headers = this[kHeaders];
+    if (typeof name !== 'string')
-    if (headers === undefined)
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
-      return;
+
-    name = String(name).trim().toLowerCase();
+    name = name.trim().toLowerCase();
-    delete headers[name];
+    delete this[kHeaders][name];
   }
 
   setHeader(name, value) {
-    name = String(name).trim().toLowerCase();
+    if (typeof name !== 'string')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
+
+    name = name.trim().toLowerCase();
     assertValidHeader(name, value);
-    let headers = this[kHeaders];
+    this[kHeaders][name] = String(value);
-    if (headers === undefined)
-      headers = this[kHeaders] = Object.create(null);
-    headers[name] = String(value);
   }
 
   get statusMessage() {
@@ -413,7 +392,8 @@ class Http2ServerResponse extends Stream {
   }
 
   flushHeaders() {
-    if (this[kStream].headersSent === false)
+    const stream = this[kStream];
+    if (stream !== undefined && stream.headersSent === false)
       this[kBeginSend]();
   }
 
@@ -427,11 +407,14 @@ class Http2ServerResponse extends Stream {
     }
 
     const stream = this[kStream];
+    if (stream === undefined) {
+      throw new errors.Error('ERR_HTTP2_STREAM_CLOSED');
+    }
     if (stream.headersSent === true) {
       throw new errors.Error('ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND');
     }
 
-    if (headers) {
+    if (typeof headers === 'object') {
       const keys = Object.keys(headers);
       let key = '';
       for (var i = 0; i < keys.length; i++) {
@@ -479,15 +462,16 @@ class Http2ServerResponse extends Stream {
       this.write(chunk, encoding);
     }
 
-    if (typeof cb === 'function' && stream !== undefined) {
+    if (stream === undefined) {
+      return;
+    }
+
+    if (typeof cb === 'function') {
       stream.once('finish', cb);
     }
 
     this[kBeginSend]({ endStream: true });
-
+    stream.end();
-    if (stream !== undefined) {
-      stream.end();
-    }
   }
 
   destroy(err) {
@@ -519,19 +503,19 @@ class Http2ServerResponse extends Stream {
 
   [kBeginSend](options) {
     const stream = this[kStream];
-    options = options || Object.create(null);
+    if (stream !== undefined &&
-    if (stream !== undefined && stream.headersSent === false) {
+        stream.destroyed === false &&
+        stream.headersSent === false) {
+      options = options || Object.create(null);
       const state = this[kState];
-      const headers = this[kHeaders] || Object.create(null);
+      const headers = this[kHeaders];
-      headers[constants.HTTP2_HEADER_STATUS] = state.statusCode;
+      headers[HTTP2_HEADER_STATUS] = state.statusCode;
       if (stream.finished === true)
         options.endStream = true;
       options.getTrailers = (trailers) => {
         Object.assign(trailers, this[kTrailers]);
       };
-      if (stream.destroyed === false) {
+      stream.respond(headers, options);
-        stream.respond(headers, options);
-      }
     }
   }
 
@@ -540,11 +524,11 @@ class Http2ServerResponse extends Stream {
     if (state.closed)
       return;
     if (code !== undefined)
-      state.closedCode = code;
+      state.closedCode = Number(code);
     state.closed = true;
     state.headersSent = this[kStream].headersSent;
     this.end();
-    this[kStream] = undefined;
+    process.nextTick(() => (this[kStream] = undefined));
     this.emit('finish');
   }
 
@@ -553,7 +537,7 @@ class Http2ServerResponse extends Stream {
     const stream = this[kStream];
     if (stream === undefined) return false;
     this[kStream].additionalHeaders({
-      [constants.HTTP2_HEADER_STATUS]: constants.HTTP_STATUS_CONTINUE
+      [HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE
     });
     return true;
   }
@@ -566,10 +550,10 @@ function onServerStream(stream, headers, flags, rawHeaders) {
   const response = new Http2ServerResponse(stream);
 
   // Check for the CONNECT method
-  const method = headers[constants.HTTP2_HEADER_METHOD];
+  const method = headers[HTTP2_HEADER_METHOD];
   if (method === 'CONNECT') {
     if (!server.emit('connect', request, response)) {
-      response.statusCode = constants.HTTP_STATUS_METHOD_NOT_ALLOWED;
+      response.statusCode = HTTP_STATUS_METHOD_NOT_ALLOWED;
       response.end();
     }
     return;
@@ -587,7 +571,7 @@ function onServerStream(stream, headers, flags, rawHeaders) {
     } else if (server.listenerCount('checkExpectation')) {
       server.emit('checkExpectation', request, response);
     } else {
-      response.statusCode = constants.HTTP_STATUS_EXPECTATION_FAILED;
+      response.statusCode = HTTP_STATUS_EXPECTATION_FAILED;
       response.end();
     }
     return;

test/parallel/test-http2-compat-serverrequest-headers.js

@@ -21,9 +21,9 @@ server.listen(0, common.mustCall(function() {
       'foo-bar': 'abc123'
     };
 
+    assert.strictEqual(request.path, undefined);
     assert.strictEqual(request.method, expected[':method']);
     assert.strictEqual(request.scheme, expected[':scheme']);
-    assert.strictEqual(request.path, expected[':path']);
     assert.strictEqual(request.url, expected[':path']);
     assert.strictEqual(request.authority, expected[':authority']);
 
@@ -41,11 +41,6 @@ server.listen(0, common.mustCall(function() {
 
     request.url = '/one';
     assert.strictEqual(request.url, '/one');
-    assert.strictEqual(request.path, '/one');
-
-    request.path = '/two';
-    assert.strictEqual(request.url, '/two');
-    assert.strictEqual(request.path, '/two');
 
     response.on('finish', common.mustCall(function() {
       server.close();

test/parallel/test-http2-compat-serverrequest.js

@@ -15,7 +15,6 @@ server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     const expected = {
-      statusCode: null,
       version: '2.0',
       httpVersionMajor: 2,
       httpVersionMinor: 0
@@ -24,7 +23,6 @@ server.listen(0, common.mustCall(function() {
     assert.strictEqual(request.closed, false);
     assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);
 
-    assert.strictEqual(request.statusCode, expected.statusCode);
     assert.strictEqual(request.httpVersion, expected.version);
     assert.strictEqual(request.httpVersionMajor, expected.httpVersionMajor);
     assert.strictEqual(request.httpVersionMinor, expected.httpVersionMinor);

test/parallel/test-http2-compat-serverresponse-destroy.js

@@ -26,7 +26,7 @@ const server = http2.createServer(common.mustCall((req, res) => {
     assert.strictEqual(res.closed, true);
   }));
 
-  if (req.path !== '/') {
+  if (req.url !== '/') {
     nextError = errors.shift();
   }
   res.destroy(nextError);

test/parallel/test-http2-compat-serverresponse-finished.js

@@ -8,13 +8,18 @@ const assert = require('assert');
 const h2 = require('http2');
 
 // Http2ServerResponse.finished
-
 const server = h2.createServer();
 server.listen(0, common.mustCall(function() {
   const port = server.address().port;
   server.once('request', common.mustCall(function(request, response) {
     response.on('finish', common.mustCall(function() {
+      assert.ok(request.stream !== undefined);
+      assert.ok(response.stream !== undefined);
       server.close();
+      process.nextTick(common.mustCall(() => {
+        assert.strictEqual(request.stream, undefined);
+        assert.strictEqual(response.stream, undefined);
+      }));
     }));
     assert.strictEqual(response.finished, false);
     response.end();

test/parallel/test-http2-compat-serverresponse-headers.js

@@ -42,6 +42,31 @@ server.listen(0, common.mustCall(function() {
     response.removeHeader(denormalised);
     assert.strictEqual(response.hasHeader(denormalised), false);
 
+    common.expectsError(
+      () => response.hasHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+    common.expectsError(
+      () => response.getHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+    common.expectsError(
+      () => response.removeHeader(),
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+
     [
       ':status',
       ':method',
@@ -70,6 +95,14 @@ server.listen(0, common.mustCall(function() {
       type: TypeError,
       message: 'Value must not be undefined or null'
     }));
+    common.expectsError(
+      () => response.setHeader(), // header name undefined
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
 
     response.setHeader(real, expectedValue);
     const expectedHeaderNames = [real];

test/parallel/test-http2-compat-serverresponse-writehead.js

@@ -22,6 +22,11 @@ server.listen(0, common.mustCall(function() {
 
     response.on('finish', common.mustCall(function() {
       server.close();
+      process.nextTick(common.mustCall(() => {
+        common.expectsError(() => { response.writeHead(300); }, {
+          code: 'ERR_HTTP2_STREAM_CLOSED'
+        });
+      }));
     }));
     response.end();
   }));

test/parallel/test-http2-createwritereq.js

@@ -30,7 +30,7 @@ const testsToRun = Object.keys(encodings).length;
 let testsFinished = 0;
 
 const server = http2.createServer(common.mustCall((req, res) => {
-  const testEncoding = encodings[req.path.slice(1)];
+  const testEncoding = encodings[req.url.slice(1)];
 
   req.on('data', common.mustCall((chunk) => assert.ok(
     Buffer.from(testString, testEncoding).equals(chunk)