From 2bcde8309cd9691b728fc0e36593dbb3d9585e78 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 4 Oct 2019 20:37:51 +0200 Subject: [PATCH] http2: allow passing FileHandle to respondWithFD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Minwoo Jung Reviewed-By: Ruben Bridgewater --- doc/api/http2.md | 9 ++- lib/fs.js | 2 +- lib/internal/fs/promises.js | 56 ++++++++++--------- lib/internal/http2/core.js | 6 +- .../test-http2-respond-file-fd-errors.js | 3 +- .../test-http2-respond-file-filehandle.js | 47 ++++++++++++++++ 6 files changed, 91 insertions(+), 32 deletions(-) create mode 100644 test/parallel/test-http2-respond-file-filehandle.js diff --git a/doc/api/http2.md b/doc/api/http2.md index 4be87eb6136230..04fdfe52167699 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1439,13 +1439,16 @@ server.on('stream', (stream) => { -* `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 4e994775a3042d..a435470fa71401 100644 --- a/lib/fs.js +++ b/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; } } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 923ae97abdc26f..9843064efbc472 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); @@ -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 }; diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 73dc6a41e9930a..28c91ee22d8f6d 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, @@ -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](); diff --git a/test/parallel/test-http2-respond-file-fd-errors.js b/test/parallel/test-http2-respond-file-fd-errors.js index 9508cfae9799c0..5de21e7855eac1 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 00000000000000..bc7bfbe356ff92 --- /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(); + })); +}));