Skip to content

Commit

Permalink
http2: allow passing FileHandle to respondWithFD
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
addaleax authored and BridgeAR committed Oct 10, 2019
1 parent adee998 commit 2bcde83
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 32 deletions.
9 changes: 6 additions & 3 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
56 changes: 30 additions & 26 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
};
6 changes: 5 additions & 1 deletion lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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]();
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-http2-respond-file-fd-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
);
Expand Down
47 changes: 47 additions & 0 deletions test/parallel/test-http2-respond-file-filehandle.js
Original file line number Diff line number Diff line change
@@ -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();
}));
}));

0 comments on commit 2bcde83

Please sign in to comment.