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>

8 files changed, 158 insertions, 138 deletions

diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js
index fa565e9bd6..04f4bb3c5a 100644
--- a/lib/internal/http2/compat.js
+++ b/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':
-      return true;
-    case ':method':
-      return true;
-    case ':path':
-      return true;
-    case ':authority':
-      return true;
-    case ':scheme':
+    case HTTP2_HEADER_STATUS:    // :status
+    case HTTP2_HEADER_METHOD:    // :method
+    case HTTP2_HEADER_PATH:      // :path
+    case HTTP2_HEADER_AUTHORITY: // :authority
+    case HTTP2_HEADER_SCHEME:    // :scheme
       return true;
     default:
       return false;
@@ -58,8 +71,7 @@ function statusMessageWarn() {
 }
 
 function onStreamData(chunk) {
-  const request = this[kRequest];
-  if (!request.push(chunk))
+  if (!this[kRequest].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];
-  request.push(null);
+  this[kRequest].push(null);
 }
 
 function onStreamError(error) {
@@ -86,13 +97,11 @@ function onStreamError(error) {
 }
 
 function onRequestPause() {
-  const stream = this[kStream];
-  stream.pause();
+  this[kStream].pause();
 }
 
 function onRequestResume() {
-  const stream = this[kStream];
-  stream.resume();
+  this[kStream].resume();
 }
 
 function onRequestDrain() {
@@ -101,13 +110,11 @@ function onRequestDrain() {
 }
 
 function onStreamResponseDrain() {
-  const response = this[kResponse];
-  response.emit('drain');
+  this[kResponse].emit('drain');
 }
 
 function onStreamClosedRequest() {
-  const req = this[kRequest];
-  req.push(null);
+  this[kRequest].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 Boolean(state.closed);
+    return this[kState].closed;
   }
 
   get code() {
-    const state = this[kState];
-    return Number(state.closedCode);
+    return this[kState].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];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_METHOD];
+    return this[kHeaders][HTTP2_HEADER_METHOD];
   }
 
   get authority() {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_AUTHORITY];
+    return this[kHeaders][HTTP2_HEADER_AUTHORITY];
   }
 
   get scheme() {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return;
-    return headers[constants.HTTP2_HEADER_SCHEME];
+    return this[kHeaders][HTTP2_HEADER_SCHEME];
   }
 
   get url() {
-    return this.path;
+    return this[kHeaders][HTTP2_HEADER_PATH];
   }
 
   set url(url) {
-    this.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;
+    this[kHeaders][HTTP2_HEADER_PATH] = url;
   }
 
   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,
-      headerCount: 0,
-      trailerCount: 0,
+      statusCode: HTTP_STATUS_OK,
       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 Boolean(state.closed);
+    return this[kState].closed;
   }
 
   get code() {
-    const state = this[kState];
-    return Number(state.closedCode);
+    return this[kState].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)
-      return;
-    if (trailers === undefined)
-      trailers = this[kTrailers] = Object.create(null);
+    let key = '';
     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 (headers === undefined)
-      return;
-    name = String(name).trim().toLowerCase();
-    return headers[name];
+    if (typeof name !== 'string')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
+
+    name = name.trim().toLowerCase();
+    return this[kHeaders][name];
   }
 
   getHeaderNames() {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return [];
-    return Object.keys(headers);
+    return Object.keys(this[kHeaders]);
   }
 
   getHeaders() {
-    const headers = this[kHeaders];
-    return Object.assign({}, headers);
+    return Object.assign({}, this[kHeaders]);
   }
 
   hasHeader(name) {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return false;
-    name = String(name).trim().toLowerCase();
-    return Object.prototype.hasOwnProperty.call(headers, name);
+    if (typeof name !== 'string')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
+
+    name = name.trim().toLowerCase();
+    return Object.prototype.hasOwnProperty.call(this[kHeaders], name);
   }
 
   removeHeader(name) {
-    const headers = this[kHeaders];
-    if (headers === undefined)
-      return;
-    name = String(name).trim().toLowerCase();
-    delete headers[name];
+    if (typeof name !== 'string')
+      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'name', 'string');
+
+    name = name.trim().toLowerCase();
+    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];
-    if (headers === undefined)
-      headers = this[kHeaders] = Object.create(null);
-    headers[name] = String(value);
+    this[kHeaders][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 });
-
-    if (stream !== undefined) {
-      stream.end();
-    }
+    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 && stream.headersSent === false) {
+    if (stream !== undefined &&
+        stream.destroyed === false &&
+        stream.headersSent === false) {
+      options = options || Object.create(null);
       const state = this[kState];
-      const headers = this[kHeaders] || Object.create(null);
-      headers[constants.HTTP2_HEADER_STATUS] = state.statusCode;
+      const headers = this[kHeaders];
+      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;
diff --git a/test/parallel/test-http2-compat-serverrequest-headers.js b/test/parallel/test-http2-compat-serverrequest-headers.js
index d5a6639105..286a0e19c8 100644
--- a/test/parallel/test-http2-compat-serverrequest-headers.js
+++ b/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();
diff --git a/test/parallel/test-http2-compat-serverrequest.js b/test/parallel/test-http2-compat-serverrequest.js
index ebc91b7a9e..a6882c6a9b 100644
--- a/test/parallel/test-http2-compat-serverrequest.js
+++ b/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);
diff --git a/test/parallel/test-http2-compat-serverresponse-destroy.js b/test/parallel/test-http2-compat-serverresponse-destroy.js
index 40c73b0887..f2b3ae7cfe 100644
--- a/test/parallel/test-http2-compat-serverresponse-destroy.js
+++ b/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);
diff --git a/test/parallel/test-http2-compat-serverresponse-finished.js b/test/parallel/test-http2-compat-serverresponse-finished.js
index c80bdc33f8..e54255f67c 100644
--- a/test/parallel/test-http2-compat-serverresponse-finished.js
+++ b/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();
diff --git a/test/parallel/test-http2-compat-serverresponse-headers.js b/test/parallel/test-http2-compat-serverresponse-headers.js
index b2285f9d2a..2cc82d4dd3 100644
--- a/test/parallel/test-http2-compat-serverresponse-headers.js
+++ b/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];
diff --git a/test/parallel/test-http2-compat-serverresponse-writehead.js b/test/parallel/test-http2-compat-serverresponse-writehead.js
index 04d7499083..12de298346 100644
--- a/test/parallel/test-http2-compat-serverresponse-writehead.js
+++ b/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();
   }));
diff --git a/test/parallel/test-http2-createwritereq.js b/test/parallel/test-http2-createwritereq.js
index 7e4bd5cf11..f4151d94e6 100644
--- a/test/parallel/test-http2-createwritereq.js
+++ b/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)