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.

Sign up for GitHub

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

Jump to bottom

fs: make open and close stream override optional when unused #40013

Merged
merged 1 commit into from
Sep 16, 2021
Merged

fs: make open and close stream override optional when unused #40013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 19 additions & 3 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1935,6 +1935,12 @@ behavior is similar to `cp dir1/ dir2/`.
<!-- YAML
added: v0.1.31
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/40013
description: The `fs` option does not need `open` method if an `fd` was provided.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/40013
description: The `fs` option does not need `close` method if `autoClose` is `false`.
- version:
- v15.4.0
pr-url: https://github.com/nodejs/node/pull/35922
Expand Down Expand Up @@ -2010,7 +2016,9 @@ destroyed, like most `Readable` streams. Set the `emitClose` option to

By providing the `fs` option, it is possible to override the corresponding `fs`
implementations for `open`, `read`, and `close`. When providing the `fs` option,
overrides for `open`, `read`, and `close` are required.
an override for `read` is required. If no `fd` is provided, an override for
`open` is also required. If `autoClose` is `true`, an override for `close` is
also required.

```mjs
import { createReadStream } from 'fs';
Expand Down Expand Up @@ -2052,6 +2060,12 @@ If `options` is a string, then it specifies the encoding.
<!-- YAML
added: v0.1.31
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/40013
description: The `fs` option does not need `open` method if an `fd` was provided.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/40013
description: The `fs` option does not need `close` method if `autoClose` is `false`.
- version:
- v15.4.0
pr-url: https://github.com/nodejs/node/pull/35922
Expand Down Expand Up @@ -2115,8 +2129,10 @@ destroyed, like most `Writable` streams. Set the `emitClose` option to
By providing the `fs` option it is possible to override the corresponding `fs`
implementations for `open`, `write`, `writev` and `close`. Overriding `write()`
without `writev()` can reduce performance as some optimizations (`_writev()`)
will be disabled. When providing the `fs` option, overrides for `open`,
`close`, and at least one of `write` and `writev` are required.
will be disabled. When providing the `fs` option, overrides for at least one of
`write` and `writev` are required. If no `fd` option is supplied, an override
for `open` is also required. If `autoClose` is `true`, an override for `close`
is also required.

Like {fs.ReadStream}, if `fd` is specified, {fs.WriteStream} will ignore the
`path` argument and will use the specified file descriptor. This means that no
Expand Down
114 changes: 58 additions & 56 deletions lib/internal/fs/streams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,29 +120,29 @@ function close(stream, err, cb) {
}

function importFd(stream, options) {
stream.fd = null;
if (options.fd != null) {
if (typeof options.fd === 'number') {
// When fd is a raw descriptor, we must keep our fingers crossed
// that the descriptor won't get closed, or worse, replaced with
// another one
// https://github.com/nodejs/node/issues/35862
stream.fd = options.fd;
} else if (typeof options.fd === 'object' &&
options.fd instanceof FileHandle) {
// When fd is a FileHandle we can listen for 'close' events
if (options.fs)
// FileHandle is not supported with custom fs operations
throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');
stream[kHandle] = options.fd;
stream.fd = options.fd.fd;
stream[kFs] = FileHandleOperations(stream[kHandle]);
stream[kHandle][kRef]();
options.fd.on('close', FunctionPrototypeBind(stream.close, stream));
} else
throw ERR_INVALID_ARG_TYPE('options.fd',
['number', 'FileHandle'], options.fd);
if (typeof options.fd === 'number') {
// When fd is a raw descriptor, we must keep our fingers crossed
// that the descriptor won't get closed, or worse, replaced with
// another one
// https://github.com/nodejs/node/issues/35862
stream[kFs] = options.fs || fs;
return options.fd;
} else if (typeof options.fd === 'object' &&
options.fd instanceof FileHandle) {
// When fd is a FileHandle we can listen for 'close' events
if (options.fs) {
// FileHandle is not supported with custom fs operations
throw new ERR_METHOD_NOT_IMPLEMENTED('FileHandle with fs');
Copy link
Member

Choose a reason for hiding this comment

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

The error message here could be more descriptive (could just use the same message as the comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error message currently is The FileHandle with fs method is not implemented. ERR_METHOD_NOT_IMPLEMENTED may not be the fittest here, but I'm afraid changing it would be a breaking change, best left to another PR. Or do you have a suggestion that would work with ERR_METHOD_NOT_IMPLEMENTED?

}
stream[kHandle] = options.fd;
stream[kFs] = FileHandleOperations(stream[kHandle]);
stream[kHandle][kRef]();
options.fd.on('close', FunctionPrototypeBind(stream.close, stream));
return options.fd.fd;
}

throw ERR_INVALID_ARG_TYPE('options.fd',
['number', 'FileHandle'], options.fd);
}

function ReadStream(path, options) {
Expand All @@ -158,21 +158,29 @@ function ReadStream(path, options) {
options.autoDestroy = false;
}

this[kFs] = options.fs || fs;
if (options.fd == null) {
this.fd = null;
this[kFs] = options.fs || fs;
validateFunction(this[kFs].open, 'options.fs.open');

validateFunction(this[kFs].open, 'options.fs.open');
validateFunction(this[kFs].read, 'options.fs.read');
validateFunction(this[kFs].close, 'options.fs.close');
// Path will be ignored when fd is specified, so it can be falsy
this.path = toPathIfFileURL(path);
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

validatePath(this.path);
} else {
this.fd = getValidatedFd(importFd(this, options));
}

options.autoDestroy = options.autoClose === undefined ?
true : options.autoClose;

// Path will be ignored when fd is specified, so it can be falsy
this.path = toPathIfFileURL(path);
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;
validateFunction(this[kFs].read, 'options.fs.read');

importFd(this, options);
if (options.autoDestroy) {
validateFunction(this[kFs].close, 'options.fs.close');
}

this.start = options.start;
this.end = options.end;
Expand All @@ -187,12 +195,6 @@ function ReadStream(path, options) {
this.pos = this.start;
}

// If fd has been set, validate, otherwise validate path.
if (this.fd != null) {
this.fd = getValidatedFd(this.fd);
} else {
validatePath(this.path);
}

if (this.end === undefined) {
this.end = Infinity;
Expand Down Expand Up @@ -310,9 +312,23 @@ function WriteStream(path, options) {
// Only buffers are supported.
options.decodeStrings = true;

this[kFs] = options.fs || fs;
if (options.fd == null) {
this.fd = null;
this[kFs] = options.fs || fs;
validateFunction(this[kFs].open, 'options.fs.open');

// Path will be ignored when fd is specified, so it can be falsy
this.path = toPathIfFileURL(path);
this.flags = options.flags === undefined ? 'w' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

validatePath(this.path);
} else {
this.fd = getValidatedFd(importFd(this, options));
}

validateFunction(this[kFs].open, 'options.fs.open');
options.autoDestroy = options.autoClose === undefined ?
true : options.autoClose;

if (!this[kFs].write && !this[kFs].writev) {
throw new ERR_INVALID_ARG_TYPE('options.fs.write', 'function',
Expand All @@ -327,7 +343,9 @@ function WriteStream(path, options) {
validateFunction(this[kFs].writev, 'options.fs.writev');
}

validateFunction(this[kFs].close, 'options.fs.close');
if (options.autoDestroy) {
validateFunction(this[kFs].close, 'options.fs.close');
}

// It's enough to override either, in which case only one will be used.
if (!this[kFs].write) {
Expand All @@ -337,28 +355,12 @@ function WriteStream(path, options) {
this._writev = null;
}

options.autoDestroy = options.autoClose === undefined ?
true : options.autoClose;

// Path will be ignored when fd is specified, so it can be falsy
this.path = toPathIfFileURL(path);
this.flags = options.flags === undefined ? 'w' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

importFd(this, options);

this.start = options.start;
this.pos = undefined;
this.bytesWritten = 0;
this.closed = false;
this[kIsPerformingIO] = false;

// If fd has been set, validate, otherwise validate path.
if (this.fd != null) {
this.fd = getValidatedFd(this.fd);
} else {
validatePath(this.path);
}

if (this.start !== undefined) {
validateInteger(this.start, 'start', 0);
Expand Down