From c7d8a424cf30b603821c4a00acfa616a4a1b0bb3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 4 Oct 2019 20:37:51 +0200 Subject: [PATCH 1/5] 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. --- doc/api/http2.md | 11 ++-- lib/fs.js | 2 +- lib/internal/fs/promises.js | 57 ++++++++++--------- lib/internal/http2/core.js | 9 ++- .../test-http2-respond-file-fd-errors.js | 3 +- .../test-http2-respond-file-filehandle.js | 47 +++++++++++++++ 6 files changed, 96 insertions(+), 33 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 d2837019bbf4cc..5b436d78c0640b 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. @@ -1540,7 +1543,7 @@ changes: regular file, is supported now. --> -* `path` {string|Buffer|URL} +* `path` {string|Buffer} * `headers` {HTTP/2 Headers Object} * `options` {Object} * `statCheck` {Function} diff --git a/lib/fs.js b/lib/fs.js index 34517a17dab484..d5c7ea70d8ec76 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 7660ff66bed9a2..4d6aea81b8d020 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,34 @@ 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, + }, + + kHandle, + FileHandle }; diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index d2701dfb84db12..22ba4130717d36 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,13 @@ class ServerHttp2Stream extends Http2Stream { this[kState].flags |= STREAM_FLAGS_HAS_TRAILERS; } - validateNumber(fd, 'fd'); + if (typeof fd !== 'number' && + !(fd instanceof fsPromisesInternal.FileHandle)) { + throw new ERR_INVALID_ARG_TYPE('fd', ['number', 'FileHandle'], fd); + } + + if (typeof fd !== 'number') // FileHandle + fd = fd.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..df06b5d0fef7de --- /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, () => { + + 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(); + }); +})); From bad2b2de84587a419659f00eb3648a4c27185829 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Oct 2019 20:29:42 +0200 Subject: [PATCH 2/5] fixup! http2: allow passing FileHandle to respondWithFD --- doc/api/http2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 5b436d78c0640b..9bb39db8215f91 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1440,7 +1440,7 @@ server.on('stream', (stream) => { added: v8.4.0 changes: - version: REPLACEME - pr-url: https://github.com/nodejs/node/pull/???? + 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 From e9d05d2de4cbe5148f8e31101177894d48a0a4b8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Oct 2019 21:58:49 +0200 Subject: [PATCH 3/5] fixup! fixup! http2: allow passing FileHandle to respondWithFD --- lib/internal/fs/promises.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 4d6aea81b8d020..31613780a78e42 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -535,6 +535,5 @@ module.exports = { readFile, }, - kHandle, FileHandle }; From 3c91a7ec624f786652710499d36e896d800b14c6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Oct 2019 22:16:27 +0200 Subject: [PATCH 4/5] fixup! http2: allow passing FileHandle to respondWithFD --- lib/internal/http2/core.js | 9 +++------ test/parallel/test-http2-respond-file-filehandle.js | 4 ++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 22ba4130717d36..11d677c5f66a31 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2524,13 +2524,10 @@ class ServerHttp2Stream extends Http2Stream { this[kState].flags |= STREAM_FLAGS_HAS_TRAILERS; } - if (typeof fd !== 'number' && - !(fd instanceof fsPromisesInternal.FileHandle)) { - throw new ERR_INVALID_ARG_TYPE('fd', ['number', 'FileHandle'], fd); - } - - if (typeof fd !== 'number') // FileHandle + 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-filehandle.js b/test/parallel/test-http2-respond-file-filehandle.js index df06b5d0fef7de..bc7bfbe356ff92 100644 --- a/test/parallel/test-http2-respond-file-filehandle.js +++ b/test/parallel/test-http2-respond-file-filehandle.js @@ -25,7 +25,7 @@ fs.promises.open(fname, 'r').then(common.mustCall((fileHandle) => { }); }); server.on('close', common.mustCall(() => fileHandle.close())); - server.listen(0, () => { + server.listen(0, common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); @@ -43,5 +43,5 @@ fs.promises.open(fname, 'r').then(common.mustCall((fileHandle) => { server.close(); })); req.end(); - }); + })); })); From cd4d52cdb36bfd6376a25136aea2bb576681322f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 Oct 2019 20:20:55 +0200 Subject: [PATCH 5/5] fixup! fixup! http2: allow passing FileHandle to respondWithFD --- doc/api/http2.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/http2.md b/doc/api/http2.md index 9bb39db8215f91..fc4b2d805ac3d5 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1543,7 +1543,7 @@ changes: regular file, is supported now. --> -* `path` {string|Buffer} +* `path` {string|Buffer|URL} * `headers` {HTTP/2 Headers Object} * `options` {Object} * `statCheck` {Function}