Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: allow passing FileHandle to respondWithFD #29876

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 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 Expand Up @@ -1540,7 +1543,7 @@ changes:
regular file, is supported now.
-->

* `path` {string|Buffer|URL}
addaleax marked this conversation as resolved.
Show resolved Hide resolved
* `path` {string|Buffer}
* `headers` {HTTP/2 Headers Object}
* `options` {Object}
* `statCheck` {Function}
Expand Down
2 changes: 1 addition & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
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 @@ -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
};
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 @@ -2523,7 +2524,10 @@ class ServerHttp2Stream extends Http2Stream {
this[kState].flags |= STREAM_FLAGS_HAS_TRAILERS;
}

validateNumber(fd, 'fd');
if (fd instanceof fsPromisesInternal.FileHandle)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't instanceof a bit slow? Maybe this world be better if the checks were the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed on request by @cjihrig … tbh, I wouldn’t expect it to make much of a difference in the big picture. And at some point in the recent past instanceof also became faster than a direct prototype comparison (according to 147b9d9), so it’s probably not too slow.

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();
}));
}));