http2: allow passing FileHandle to respondWithFD

This seems to make sense if we want to promote the use
of `fs.promises`, although it’s not strictly necessary.

PR-URL: #29876
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

Author: addaleax

doc/api/http2.md

@@ -1439,13 +1439,16 @@ server.on('stream', (stream) => {
 <!-- YAML
 added: v8.4.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/29876
+    description: The `fd` option may now be a `FileHandle`.
   - version: v10.0.0
     pr-url: https://github.com/nodejs/node/pull/18936
     description: Any readable file descriptor, not necessarily for a
                  regular file, is supported now.
 -->
 
-* `fd` {number} A readable file descriptor.
+* `fd` {number|FileHandle} A readable file descriptor.
 * `headers` {HTTP/2 Headers Object}
 * `options` {Object}
   * `statCheck` {Function}
@@ -1491,8 +1494,8 @@ The `offset` and `length` options may be used to limit the response to a
 specific range subset. This can be used, for instance, to support HTTP Range
 requests.
 
-The file descriptor is not closed when the stream is closed, so it will need
-to be closed manually once it is no longer needed.
+The file descriptor or `FileHandle` is not closed when the stream is closed,
+so it will need to be closed manually once it is no longer needed.
 Using the same file descriptor concurrently for multiple streams
 is not supported and may result in data loss. Re-using a file descriptor
 after a stream has finished is supported.

lib/fs.js

@@ -1948,7 +1948,7 @@ Object.defineProperties(fs, {
     enumerable: true,
     get() {
       if (promises === null)
-        promises = require('internal/fs/promises');
+        promises = require('internal/fs/promises').exports;
       return promises;
     }
   }

lib/internal/fs/promises.js

@@ -46,7 +46,7 @@ const {
 const pathModule = require('path');
 const { promisify } = require('internal/util');
 
-const kHandle = Symbol('handle');
+const kHandle = Symbol('kHandle');
 const { kUsePromises } = binding;
 
 const getDirectoryEntriesPromise = promisify(getDirents);
@@ -498,29 +498,33 @@ async function readFile(path, options) {
 }
 
 module.exports = {
-  access,
-  copyFile,
-  open,
-  opendir: promisify(opendir),
-  rename,
-  truncate,
-  rmdir,
-  mkdir,
-  readdir,
-  readlink,
-  symlink,
-  lstat,
-  stat,
-  link,
-  unlink,
-  chmod,
-  lchmod,
-  lchown,
-  chown,
-  utimes,
-  realpath,
-  mkdtemp,
-  writeFile,
-  appendFile,
-  readFile
+  exports: {
+    access,
+    copyFile,
+    open,
+    opendir: promisify(opendir),
+    rename,
+    truncate,
+    rmdir,
+    mkdir,
+    readdir,
+    readlink,
+    symlink,
+    lstat,
+    stat,
+    link,
+    unlink,
+    chmod,
+    lchmod,
+    lchown,
+    chown,
+    utimes,
+    realpath,
+    mkdtemp,
+    writeFile,
+    appendFile,
+    readFile,
+  },
+
+  FileHandle
 };

lib/internal/http2/core.js

@@ -82,6 +82,7 @@ const {
   hideStackFrames
 } = require('internal/errors');
 const { validateNumber, validateString } = require('internal/validators');
+const fsPromisesInternal = require('internal/fs/promises');
 const { utcDate } = require('internal/http');
 const { onServerStream,
         Http2ServerRequest,
@@ -2545,7 +2546,10 @@ class ServerHttp2Stream extends Http2Stream {
       this[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
     }
 
-    validateNumber(fd, 'fd');
+    if (fd instanceof fsPromisesInternal.FileHandle)
+      fd = fd.fd;
+    else if (typeof fd !== 'number')
+      throw new ERR_INVALID_ARG_TYPE('fd', ['number', 'FileHandle'], fd);
 
     debugStreamObj(this, 'initiating response from fd');
     this[kUpdateTimer]();

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

@@ -42,7 +42,8 @@ server.on('stream', common.mustCall((stream) => {
       {
         type: TypeError,
         code: 'ERR_INVALID_ARG_TYPE',
-        message: 'The "fd" argument must be of type number. Received type ' +
+        message: 'The "fd" argument must be one of type number or FileHandle.' +
+                 ' Received type ' +
                  typeof types[type]
       }
     );

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

@@ -0,0 +1,47 @@
+'use strict';
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+const assert = require('assert');
+const fs = require('fs');
+
+const {
+  HTTP2_HEADER_CONTENT_TYPE,
+  HTTP2_HEADER_CONTENT_LENGTH
+} = http2.constants;
+
+const fname = fixtures.path('elipses.txt');
+const data = fs.readFileSync(fname);
+const stat = fs.statSync(fname);
+fs.promises.open(fname, 'r').then(common.mustCall((fileHandle) => {
+  const server = http2.createServer();
+  server.on('stream', (stream) => {
+    stream.respondWithFD(fileHandle, {
+      [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
+      [HTTP2_HEADER_CONTENT_LENGTH]: stat.size,
+    });
+  });
+  server.on('close', common.mustCall(() => fileHandle.close()));
+  server.listen(0, common.mustCall(() => {
+
+    const client = http2.connect(`http://localhost:${server.address().port}`);
+    const req = client.request();
+
+    req.on('response', common.mustCall((headers) => {
+      assert.strictEqual(headers[HTTP2_HEADER_CONTENT_TYPE], 'text/plain');
+      assert.strictEqual(+headers[HTTP2_HEADER_CONTENT_LENGTH], data.length);
+    }));
+    req.setEncoding('utf8');
+    let check = '';
+    req.on('data', (chunk) => check += chunk);
+    req.on('end', common.mustCall(() => {
+      assert.strictEqual(check, data.toString('utf8'));
+      client.close();
+      server.close();
+    }));
+    req.end();
+  }));
+}));