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: https://github.com/nodejs/node/pull/29876
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

6 files changed, 91 insertions, 32 deletions

diff --git a/doc/api/http2.md b/doc/api/http2.md
index d2837019bb..fc4b2d805a 100644
--- a/doc/api/http2.md
+++ b/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.
diff --git a/lib/fs.js b/lib/fs.js
index 34517a17da..d5c7ea70d8 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1964,7 +1964,7 @@ Object.defineProperties(fs, {
     enumerable: true,
     get() {
       if (promises === null)
-        promises = require('internal/fs/promises');
+        promises = require('internal/fs/promises').exports;
       return promises;
     }
   }
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 7660ff66be..31613780a7 100644
--- a/lib/internal/fs/promises.js
+++ b/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);
@@ -507,29 +507,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
 };
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index d2701dfb84..11d677c5f6 100644
--- a/lib/internal/http2/core.js
+++ b/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,
@@ -2523,7 +2524,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]();
diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js
index 9508cfae97..5de21e7855 100644
--- a/test/parallel/test-http2-respond-file-fd-errors.js
+++ b/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]
       }
     );
diff --git a/test/parallel/test-http2-respond-file-filehandle.js b/test/parallel/test-http2-respond-file-filehandle.js
new file mode 100644
index 0000000000..bc7bfbe356
--- /dev/null
+++ b/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();
+  }));
+}));