Skip to content

Commit

Permalink
fs: make open and close stream override optional when unused
Browse files Browse the repository at this point in the history 
When using `createReadStream` or `createWriteStream` with a specific
file descriptor or `FileHandle` instead of a path, `open` method is not
used, there is no point in forcing users to provide it.
When using `createReadStream` or `createWriteStream` with  `autoClose`
set to false, `close` method is not used, there is no point in forcing
users to provide it.

PR-URL: #40013
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@aduh95 @BethGriggs
aduh95 authored and BethGriggs committed Sep 21, 2021
1 parent dcf279b commit e86a813
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 59 deletions.
22 changes: 19 additions & 3 deletions doc/api/fs.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1887,6 +1887,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 @@ -1962,7 +1968,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 @@ -2004,6 +2012,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 @@ -2067,8 +2081,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
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');
}
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

0 comments on commit e86a813

Please sign in to comment.