http2: small fixes to compatibility layer

Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: https://github.com/nodejs/node/pull/15473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

lib/internal/http2/compat.js

@@ -92,7 +92,7 @@ function onStreamError(error) {
   // errors in compatibility mode are
   // not forwarded to the request
   // and response objects. However,
-  // they are forwarded to 'clientError'
+  // they are forwarded to 'streamError'
   // on the server by Http2Stream
 }
 
@@ -248,9 +248,9 @@ class Http2ServerRequest extends Readable {
   }
 
   setTimeout(msecs, callback) {
-    const stream = this[kStream];
-    if (stream === undefined) return;
-    stream.setTimeout(msecs, callback);
+    if (this[kState].closed)
+      return;
+    this[kStream].setTimeout(msecs, callback);
   }
 
   [kFinish](code) {
@@ -445,7 +445,7 @@ class Http2ServerResponse extends Stream {
 
     if (stream === undefined) {
       const err = new errors.Error('ERR_HTTP2_STREAM_CLOSED');
-      if (cb)
+      if (typeof cb === 'function')
         process.nextTick(cb, err);
       else
         throw err;
@@ -461,12 +461,11 @@ class Http2ServerResponse extends Stream {
     if (typeof chunk === 'function') {
       cb = chunk;
       chunk = null;
-      encoding = 'utf8';
     } else if (typeof encoding === 'function') {
       cb = encoding;
       encoding = 'utf8';
     }
-    if (stream === undefined || stream.finished === true) {
+    if (this.finished === true) {
       return false;
     }
     if (chunk !== null && chunk !== undefined) {
@@ -482,21 +481,21 @@ class Http2ServerResponse extends Stream {
   }
 
   destroy(err) {
-    const stream = this[kStream];
-    if (stream === undefined) {
-      // nothing to do, already closed
+    if (this[kState].closed)
       return;
-    }
-    stream.destroy(err);
+    this[kStream].destroy(err);
   }
 
   setTimeout(msecs, callback) {
     const stream = this[kStream];
-    if (stream === undefined) return;
+    if (this[kState].closed)
+      return;
     stream.setTimeout(msecs, callback);
   }
 
   createPushResponse(headers, callback) {
+    if (typeof callback !== 'function')
+      throw new errors.TypeError('ERR_INVALID_CALLBACK');
     const stream = this[kStream];
     if (stream === undefined) {
       process.nextTick(callback, new errors.Error('ERR_HTTP2_STREAM_CLOSED'));
@@ -513,12 +512,9 @@ class Http2ServerResponse extends Stream {
     if (stream !== undefined &&
         stream.destroyed === false &&
         stream.headersSent === false) {
-      options = options || Object.create(null);
-      const state = this[kState];
       const headers = this[kHeaders];
-      headers[HTTP2_HEADER_STATUS] = state.statusCode;
-      if (stream.finished === true)
-        options.endStream = true;
+      headers[HTTP2_HEADER_STATUS] = this[kState].statusCode;
+      options = options || Object.create(null);
       options.getTrailers = (trailers) => {
         Object.assign(trailers, this[kTrailers]);
       };
@@ -542,7 +538,11 @@ class Http2ServerResponse extends Stream {
   // TODO doesn't support callbacks
   writeContinue() {
     const stream = this[kStream];
-    if (stream === undefined) return false;
+    if (stream === undefined ||
+        stream.headersSent === true ||
+        stream.destroyed === true) {
+      return false;
+    }
     this[kStream].additionalHeaders({
       [HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE
     });

test/parallel/test-http2-compat-expect-continue-check.js

@@ -21,6 +21,11 @@ function handler(req, res) {
     'abcd': '1'
   });
   res.end(testResBody);
+  // should simply return false if already too late to write
+  assert.strictEqual(res.writeContinue(), false);
+  res.on('finish', common.mustCall(
+    () => process.nextTick(() => assert.strictEqual(res.writeContinue(), false))
+  ));
 }
 
 const server = http2.createServer(

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

@@ -4,13 +4,25 @@
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
+const assert = require('assert');
 const http2 = require('http2');
 
+const msecs = common.platformTimeout(1);
 const server = http2.createServer();
 
 server.on('request', (req, res) => {
-  req.setTimeout(common.platformTimeout(1), common.mustCall(() => {
+  req.setTimeout(msecs, common.mustCall(() => {
     res.end();
+    req.setTimeout(msecs, common.mustNotCall());
+  }));
+  res.on('finish', common.mustCall(() => {
+    req.setTimeout(msecs, common.mustNotCall());
+    process.nextTick(() => {
+      assert.doesNotThrow(
+        () => req.setTimeout(msecs, common.mustNotCall())
+      );
+      server.close();
+    });
   }));
 });
 
@@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => {
     ':authority': `localhost:${port}`
   });
   req.on('end', common.mustCall(() => {
-    server.close();
     client.destroy();
   }));
   req.resume();

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

@@ -34,7 +34,10 @@ server.listen(0, common.mustCall(function() {
     response.on('finish', common.mustCall(function() {
       assert.strictEqual(request.closed, true);
       assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);
-      server.close();
+      process.nextTick(() => {
+        assert.strictEqual(request.socket, undefined);
+        server.close();
+      });
     }));
     response.end();
   }));

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

@@ -16,6 +16,19 @@ const server = h2.createServer((request, response) => {
   assert.strictEqual(response.stream.id % 2, 1);
   response.write(servExpect);
 
+  // callback must be specified (and be a function)
+  common.expectsError(
+    () => response.createPushResponse({
+      ':path': '/pushed',
+      ':method': 'GET'
+    }, undefined),
+    {
+      code: 'ERR_INVALID_CALLBACK',
+      type: TypeError,
+      message: 'Callback must be a function'
+    }
+  );
+
   response.createPushResponse({
     ':path': '/pushed',
     ':method': 'GET'

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

@@ -24,6 +24,9 @@ const server = http2.createServer(common.mustCall((req, res) => {
   res.on('finish', common.mustCall(() => {
     assert.doesNotThrow(() => res.destroy(nextError));
     assert.strictEqual(res.closed, true);
+    process.nextTick(() => {
+      assert.doesNotThrow(() => res.destroy(nextError));
+    });
   }));
 
   if (req.url !== '/') {

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

@@ -4,7 +4,7 @@
 const { mustCall, mustNotCall, hasCrypto, skip } = require('../common');
 if (!hasCrypto)
   skip('missing crypto');
-const { doesNotThrow, strictEqual } = require('assert');
+const { strictEqual } = require('assert');
 const {
   createServer,
   connect,
@@ -15,18 +15,82 @@ const {
 } = require('http2');
 
 {
-  // Http2ServerResponse.end callback is called only the first time,
-  // but may be invoked repeatedly without throwing errors.
+  // Http2ServerResponse.end accepts chunk, encoding, cb as args
+  // It may be invoked repeatedly without throwing errors
+  // but callback will only be called once
   const server = createServer(mustCall((request, response) => {
     strictEqual(response.closed, false);
-    response.on('finish', mustCall(() => process.nextTick(
-      mustCall(() => doesNotThrow(() => response.end('test', mustNotCall())))
-    )));
+    response.end('end', 'utf8', mustCall(() => {
+      strictEqual(response.closed, true);
+      response.end(mustNotCall());
+      process.nextTick(() => {
+        response.end(mustNotCall());
+        server.close();
+      });
+    }));
+    response.end(mustNotCall());
+  }));
+  server.listen(0, mustCall(() => {
+    let data = '';
+    const { port } = server.address();
+    const url = `http://localhost:${port}`;
+    const client = connect(url, mustCall(() => {
+      const headers = {
+        ':path': '/',
+        ':method': 'GET',
+        ':scheme': 'http',
+        ':authority': `localhost:${port}`
+      };
+      const request = client.request(headers);
+      request.setEncoding('utf8');
+      request.on('data', (chunk) => (data += chunk));
+      request.on('end', mustCall(() => {
+        strictEqual(data, 'end');
+        client.destroy();
+      }));
+      request.end();
+      request.resume();
+    }));
+  }));
+}
+
+{
+  // Http2ServerResponse.end can omit encoding arg, sets it to utf-8
+  const server = createServer(mustCall((request, response) => {
+    response.end('test\uD83D\uDE00', mustCall(() => {
+      server.close();
+    }));
+  }));
+  server.listen(0, mustCall(() => {
+    let data = '';
+    const { port } = server.address();
+    const url = `http://localhost:${port}`;
+    const client = connect(url, mustCall(() => {
+      const headers = {
+        ':path': '/',
+        ':method': 'GET',
+        ':scheme': 'http',
+        ':authority': `localhost:${port}`
+      };
+      const request = client.request(headers);
+      request.setEncoding('utf8');
+      request.on('data', (chunk) => (data += chunk));
+      request.on('end', mustCall(() => {
+        strictEqual(data, 'test\uD83D\uDE00');
+        client.destroy();
+      }));
+      request.end();
+      request.resume();
+    }));
+  }));
+}
+
+{
+  // Http2ServerResponse.end can omit chunk & encoding args
+  const server = createServer(mustCall((request, response) => {
     response.end(mustCall(() => {
       server.close();
     }));
-    response.end(mustNotCall());
-    strictEqual(response.closed, true);
   }));
   server.listen(0, mustCall(() => {
     const { port } = server.address();

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

@@ -103,6 +103,14 @@ server.listen(0, common.mustCall(function() {
         message: 'The "name" argument must be of type string'
       }
     );
+    common.expectsError(
+      () => response.setHeader(''),
+      {
+        code: 'ERR_INVALID_HTTP_TOKEN',
+        type: TypeError,
+        message: 'Header name must be a valid HTTP token [""]'
+      }
+    );
 
     response.setHeader(real, expectedValue);
     const expectedHeaderNames = [real];
@@ -122,7 +130,12 @@ server.listen(0, common.mustCall(function() {
     response.on('finish', common.mustCall(function() {
       assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR);
       assert.strictEqual(response.headersSent, true);
-      server.close();
+      process.nextTick(() => {
+        // can access headersSent after stream is undefined
+        assert.strictEqual(response.stream, undefined);
+        assert.strictEqual(response.headersSent, true);
+        server.close();
+      });
     }));
     response.end();
   }));

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

@@ -4,13 +4,25 @@
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
+const assert = require('assert');
 const http2 = require('http2');
 
+const msecs = common.platformTimeout(1);
 const server = http2.createServer();
 
 server.on('request', (req, res) => {
-  res.setTimeout(common.platformTimeout(1), common.mustCall(() => {
+  res.setTimeout(msecs, common.mustCall(() => {
     res.end();
+    res.setTimeout(msecs, common.mustNotCall());
+  }));
+  res.on('finish', common.mustCall(() => {
+    res.setTimeout(msecs, common.mustNotCall());
+    process.nextTick(() => {
+      assert.doesNotThrow(
+        () => res.setTimeout(msecs, common.mustNotCall())
+      );
+      server.close();
+    });
   }));
 });
 
@@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => {
     ':authority': `localhost:${port}`
   });
   req.on('end', common.mustCall(() => {
-    server.close();
     client.destroy();
   }));
   req.resume();

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

@@ -14,6 +14,49 @@ server.listen(0, common.mustCall(() => {
     response.addTrailers({
       ABC: 123
     });
+    response.setTrailer('ABCD', 123);
+
+    common.expectsError(
+      () => response.addTrailers({ '': 'test' }),
+      {
+        code: 'ERR_INVALID_HTTP_TOKEN',
+        type: TypeError,
+        message: 'Header name must be a valid HTTP token [""]'
+      }
+    );
+    common.expectsError(
+      () => response.setTrailer('test', undefined),
+      {
+        code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
+        type: TypeError,
+        message: 'Value must not be undefined or null'
+      }
+    );
+    common.expectsError(
+      () => response.setTrailer('test', null),
+      {
+        code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
+        type: TypeError,
+        message: 'Value must not be undefined or null'
+      }
+    );
+    common.expectsError(
+      () => response.setTrailer(), // trailer name undefined
+      {
+        code: 'ERR_INVALID_ARG_TYPE',
+        type: TypeError,
+        message: 'The "name" argument must be of type string'
+      }
+    );
+    common.expectsError(
+      () => response.setTrailer(''),
+      {
+        code: 'ERR_INVALID_HTTP_TOKEN',
+        type: TypeError,
+        message: 'Header name must be a valid HTTP token [""]'
+      }
+    );
+
     response.end('hello');
   }));
 
@@ -22,6 +65,7 @@ server.listen(0, common.mustCall(() => {
     const request = client.request();
     request.on('trailers', common.mustCall((headers) => {
       assert.strictEqual(headers.abc, '123');
+      assert.strictEqual(headers.abcd, '123');
     }));
     request.resume();
     request.on('end', common.mustCall(() => {