http2: fix refs to status 205, add tests

Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point
to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204,
205 & 304 in respond, respondWithFD & respondWithFile. Add general
error tests for respondWithFD & respondWithFile.

PR-URL: https://github.com/nodejs/node/pull/15153
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

lib/internal/http2/core.js

@@ -121,7 +121,7 @@ const {
   HTTP2_METHOD_CONNECT,
 
   HTTP_STATUS_CONTINUE,
-  HTTP_STATUS_CONTENT_RESET,
+  HTTP_STATUS_RESET_CONTENT,
   HTTP_STATUS_OK,
   HTTP_STATUS_NO_CONTENT,
   HTTP_STATUS_NOT_MODIFIED,
@@ -1879,7 +1879,7 @@ class ServerHttp2Stream extends Http2Stream {
     // the options.endStream option to true so that the underlying
     // bits do not attempt to send any.
     if (statusCode === HTTP_STATUS_NO_CONTENT ||
-        statusCode === HTTP_STATUS_CONTENT_RESET ||
+        statusCode === HTTP_STATUS_RESET_CONTENT ||
         statusCode === HTTP_STATUS_NOT_MODIFIED ||
         state.headRequest === true) {
       options.endStream = true;
@@ -1973,7 +1973,7 @@ class ServerHttp2Stream extends Http2Stream {
     const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
     // Payload/DATA frames are not permitted in these cases
     if (statusCode === HTTP_STATUS_NO_CONTENT ||
-        statusCode === HTTP_STATUS_CONTENT_RESET ||
+        statusCode === HTTP_STATUS_RESET_CONTENT ||
         statusCode === HTTP_STATUS_NOT_MODIFIED) {
       throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode);
     }
@@ -2050,7 +2050,7 @@ class ServerHttp2Stream extends Http2Stream {
     const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
     // Payload/DATA frames are not permitted in these cases
     if (statusCode === HTTP_STATUS_NO_CONTENT ||
-        statusCode === HTTP_STATUS_CONTENT_RESET ||
+        statusCode === HTTP_STATUS_RESET_CONTENT ||
         statusCode === HTTP_STATUS_NOT_MODIFIED) {
       throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode);
     }

test/parallel/test-http2-respond-file-errors.js

@@ -0,0 +1,103 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const path = require('path');
+
+const optionsWithTypeError = {
+  offset: 'number',
+  length: 'number',
+  statCheck: 'function',
+  getTrailers: 'function'
+};
+
+const types = {
+  boolean: true,
+  function: () => {},
+  number: 1,
+  object: {},
+  array: [],
+  null: null,
+  symbol: Symbol('test')
+};
+
+const fname = path.resolve(common.fixturesDir, 'elipses.txt');
+
+const server = http2.createServer();
+
+server.on('stream', common.mustCall((stream) => {
+  // Check for all possible TypeError triggers on options
+  Object.keys(optionsWithTypeError).forEach((option) => {
+    Object.keys(types).forEach((type) => {
+      if (type === optionsWithTypeError[option]) {
+        return;
+      }
+
+      common.expectsError(
+        () => stream.respondWithFile(fname, {
+          [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+        }, {
+          [option]: types[type]
+        }),
+        {
+          type: TypeError,
+          code: 'ERR_INVALID_OPT_VALUE',
+          message: `The value "${String(types[type])}" is invalid ` +
+                   `for option "${option}"`
+        }
+      );
+    });
+  });
+
+  // Should throw if :status 204, 205 or 304
+  [204, 205, 304].forEach((status) => common.expectsError(
+    () => stream.respondWithFile(fname, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
+      ':status': status,
+    }),
+    {
+      code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN',
+      message: `Responses with ${status} status must not have a payload`
+    }
+  ));
+
+  // Should throw if headers already sent
+  stream.respond({
+    ':status': 200,
+  });
+  common.expectsError(
+    () => stream.respondWithFile(fname, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+    }),
+    {
+      code: 'ERR_HTTP2_HEADERS_SENT',
+      message: 'Response has already been initiated.'
+    }
+  );
+
+  // Should throw if stream already destroyed
+  stream.destroy();
+  common.expectsError(
+    () => stream.respondWithFile(fname, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+    }),
+    {
+      code: 'ERR_HTTP2_INVALID_STREAM',
+      message: 'The stream has been destroyed'
+    }
+  );
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('streamClosed', common.mustCall(() => {
+    client.destroy();
+    server.close();
+  }));
+  req.end();
+}));

test/parallel/test-http2-respond-file-fd-errors.js

@@ -0,0 +1,123 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const path = require('path');
+const fs = require('fs');
+
+const optionsWithTypeError = {
+  offset: 'number',
+  length: 'number',
+  statCheck: 'function',
+  getTrailers: 'function'
+};
+
+const types = {
+  boolean: true,
+  function: () => {},
+  number: 1,
+  object: {},
+  array: [],
+  null: null,
+  symbol: Symbol('test')
+};
+
+const fname = path.resolve(common.fixturesDir, 'elipses.txt');
+const fd = fs.openSync(fname, 'r');
+
+const server = http2.createServer();
+
+server.on('stream', common.mustCall((stream) => {
+  // should throw if fd isn't a number
+  Object.keys(types).forEach((type) => {
+    if (type === 'number') {
+      return;
+    }
+
+    common.expectsError(
+      () => stream.respondWithFD(types[type], {
+        [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+      }),
+      {
+        type: TypeError,
+        code: 'ERR_INVALID_ARG_TYPE',
+        message: 'The "fd" argument must be of type number'
+      }
+    );
+  });
+
+  // Check for all possible TypeError triggers on options
+  Object.keys(optionsWithTypeError).forEach((option) => {
+    Object.keys(types).forEach((type) => {
+      if (type === optionsWithTypeError[option]) {
+        return;
+      }
+
+      common.expectsError(
+        () => stream.respondWithFD(fd, {
+          [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+        }, {
+          [option]: types[type]
+        }),
+        {
+          type: TypeError,
+          code: 'ERR_INVALID_OPT_VALUE',
+          message: `The value "${String(types[type])}" is invalid ` +
+                   `for option "${option}"`
+        }
+      );
+    });
+  });
+
+  // Should throw if :status 204, 205 or 304
+  [204, 205, 304].forEach((status) => common.expectsError(
+    () => stream.respondWithFD(fd, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
+      ':status': status,
+    }),
+    {
+      code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN',
+      message: `Responses with ${status} status must not have a payload`
+    }
+  ));
+
+  // Should throw if headers already sent
+  stream.respond({
+    ':status': 200,
+  });
+  common.expectsError(
+    () => stream.respondWithFD(fd, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+    }),
+    {
+      code: 'ERR_HTTP2_HEADERS_SENT',
+      message: 'Response has already been initiated.'
+    }
+  );
+
+  // Should throw if stream already destroyed
+  stream.destroy();
+  common.expectsError(
+    () => stream.respondWithFD(fd, {
+      [http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
+    }),
+    {
+      code: 'ERR_HTTP2_INVALID_STREAM',
+      message: 'The stream has been destroyed'
+    }
+  );
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+
+  req.on('streamClosed', common.mustCall(() => {
+    client.destroy();
+    server.close();
+  }));
+  req.end();
+}));

test/parallel/test-http2-respond-no-data.js

@@ -0,0 +1,40 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const assert = require('assert');
+
+const server = http2.createServer();
+
+// Check that stream ends immediately after respond on :status 204, 205 & 304
+
+const status = [204, 205, 304];
+
+server.on('stream', common.mustCall((stream) => {
+  stream.on('streamClosed', common.mustCall(() => {
+    assert.strictEqual(stream.destroyed, true);
+  }));
+  stream.respond({ ':status': status.shift() });
+}, 3));
+
+server.listen(0, common.mustCall(makeRequest));
+
+function makeRequest() {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request();
+  req.resume();
+
+  req.on('end', common.mustCall(() => {
+    client.destroy();
+
+    if (!status.length) {
+      server.close();
+    } else {
+      makeRequest();
+    }
+  }));
+  req.end();
+}