Skip to content

Commit 0ff4a55

Browse files
addaleaxaddaleax
committed
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]>
1 parent a04b04f commit 0ff4a55

File tree

6 files changed

+91
-32
lines changed

6 files changed

+91
-32
lines changed

‎doc/api/http2.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1439,13 +1439,16 @@ server.on('stream', (stream) => {
14391439
<!-- YAML
14401440
added: v8.4.0
14411441
changes:
1442+
- version: REPLACEME
1443+
pr-url: https://github.com/nodejs/node/pull/29876
1444+
description: The `fd` option may now be a `FileHandle`.
14421445
- version: v10.0.0
14431446
pr-url: https://github.com/nodejs/node/pull/18936
14441447
description: Any readable file descriptor, not necessarily for a
14451448
regular file, is supported now.
14461449
-->
14471450

1448-
* `fd` {number} A readable file descriptor.
1451+
* `fd` {number|FileHandle} A readable file descriptor.
14491452
* `headers` {HTTP/2 Headers Object}
14501453
* `options` {Object}
14511454
* `statCheck` {Function}
@@ -1491,8 +1494,8 @@ The `offset` and `length` options may be used to limit the response to a
14911494
specific range subset. This can be used, for instance, to support HTTP Range
14921495
requests.
14931496

1494-
The file descriptor is not closed when the stream is closed, so it will need
1495-
to be closed manually once it is no longer needed.
1497+
The file descriptor or `FileHandle` is not closed when the stream is closed,
1498+
so it will need to be closed manually once it is no longer needed.
14961499
Using the same file descriptor concurrently for multiple streams
14971500
is not supported and may result in data loss. Re-using a file descriptor
14981501
after a stream has finished is supported.

‎lib/fs.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1964,7 +1964,7 @@ Object.defineProperties(fs, {
19641964
enumerable: true,
19651965
get() {
19661966
if (promises === null)
1967-
promises = require('internal/fs/promises');
1967+
promises = require('internal/fs/promises').exports;
19681968
return promises;
19691969
}
19701970
}

‎lib/internal/fs/promises.js‎

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ const {
4646
const pathModule = require('path');
4747
const { promisify } = require('internal/util');
4848

49-
const kHandle = Symbol('handle');
49+
const kHandle = Symbol('kHandle');
5050
const { kUsePromises } = binding;
5151

5252
const getDirectoryEntriesPromise = promisify(getDirents);
@@ -507,29 +507,33 @@ async function readFile(path, options) {
507507
}
508508

509509
module.exports = {
510-
access,
511-
copyFile,
512-
open,
513-
opendir: promisify(opendir),
514-
rename,
515-
truncate,
516-
rmdir,
517-
mkdir,
518-
readdir,
519-
readlink,
520-
symlink,
521-
lstat,
522-
stat,
523-
link,
524-
unlink,
525-
chmod,
526-
lchmod,
527-
lchown,
528-
chown,
529-
utimes,
530-
realpath,
531-
mkdtemp,
532-
writeFile,
533-
appendFile,
534-
readFile
510+
exports: {
511+
access,
512+
copyFile,
513+
open,
514+
opendir: promisify(opendir),
515+
rename,
516+
truncate,
517+
rmdir,
518+
mkdir,
519+
readdir,
520+
readlink,
521+
symlink,
522+
lstat,
523+
stat,
524+
link,
525+
unlink,
526+
chmod,
527+
lchmod,
528+
lchown,
529+
chown,
530+
utimes,
531+
realpath,
532+
mkdtemp,
533+
writeFile,
534+
appendFile,
535+
readFile,
536+
},
537+
538+
FileHandle
535539
};

‎lib/internal/http2/core.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ const {
8282
hideStackFrames
8383
} = require('internal/errors');
8484
const { validateNumber, validateString } = require('internal/validators');
85+
const fsPromisesInternal = require('internal/fs/promises');
8586
const { utcDate } = require('internal/http');
8687
const { onServerStream,
8788
Http2ServerRequest,
@@ -2523,7 +2524,10 @@ class ServerHttp2Stream extends Http2Stream {
25232524
this[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
25242525
}
25252526

2526-
validateNumber(fd, 'fd');
2527+
if (fd instanceof fsPromisesInternal.FileHandle)
2528+
fd = fd.fd;
2529+
else if (typeof fd !== 'number')
2530+
throw new ERR_INVALID_ARG_TYPE('fd', ['number', 'FileHandle'], fd);
25272531

25282532
debugStreamObj(this, 'initiating response from fd');
25292533
this[kUpdateTimer]();

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ server.on('stream', common.mustCall((stream) => {
4242
{
4343
type: TypeError,
4444
code: 'ERR_INVALID_ARG_TYPE',
45-
message: 'The "fd" argument must be of type number. Received type ' +
45+
message: 'The "fd" argument must be one of type number or FileHandle.' +
46+
' Received type ' +
4647
typeof types[type]
4748
}
4849
);

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fixtures = require('../common/fixtures');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const http2 = require('http2');
8+
const assert = require('assert');
9+
const fs = require('fs');
10+
11+
const {
12+
HTTP2_HEADER_CONTENT_TYPE,
13+
HTTP2_HEADER_CONTENT_LENGTH
14+
} = http2.constants;
15+
16+
const fname = fixtures.path('elipses.txt');
17+
const data = fs.readFileSync(fname);
18+
const stat = fs.statSync(fname);
19+
fs.promises.open(fname, 'r').then(common.mustCall((fileHandle) => {
20+
const server = http2.createServer();
21+
server.on('stream', (stream) => {
22+
stream.respondWithFD(fileHandle, {
23+
[HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
24+
[HTTP2_HEADER_CONTENT_LENGTH]: stat.size,
25+
});
26+
});
27+
server.on('close', common.mustCall(() => fileHandle.close()));
28+
server.listen(0, common.mustCall(() => {
29+
30+
const client = http2.connect(`http://localhost:${server.address().port}`);
31+
const req = client.request();
32+
33+
req.on('response', common.mustCall((headers) => {
34+
assert.strictEqual(headers[HTTP2_HEADER_CONTENT_TYPE], 'text/plain');
35+
assert.strictEqual(+headers[HTTP2_HEADER_CONTENT_LENGTH], data.length);
36+
}));
37+
req.setEncoding('utf8');
38+
let check = '';
39+
req.on('data', (chunk) => check += chunk);
40+
req.on('end', common.mustCall(() => {
41+
assert.strictEqual(check, data.toString('utf8'));
42+
client.close();
43+
server.close();
44+
}));
45+
req.end();
46+
}));
47+
}));

0 commit comments

Comments
 (0)