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

fs: support abortsignal in writeFile #35993

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
61 changes: 60 additions & 1 deletion doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -4385,6 +4385,10 @@ details.
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/35993
description: The options argument may include an AbortSignal to abort an
ongoing writeFile request.
- version: v14.12.0
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
Expand Down Expand Up @@ -4419,6 +4423,7 @@ changes:
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
* `signal` {AbortSignal} allows aborting an in-progress writeFile
* `callback` {Function}
* `err` {Error}

Expand Down Expand Up @@ -4450,6 +4455,28 @@ It is unsafe to use `fs.writeFile()` multiple times on the same file without
waiting for the callback. For this scenario, [`fs.createWriteStream()`][] is
recommended.

Similarly to `fs.readFile` - `fs.writeFile` is a convenience method that
performs multiple `write` calls internally to write the buffer passed to it.
For performance sensitive code consider using [`fs.createWriteStream()`][].

It is possible to use an {AbortSignal} to cancel an `fs.writeFile()`.
Cancelation is "best effort", and some amount of data is likely still
to be written.

```js
const controller = new AbortController();
const { signal } = controller;
const data = new Uint8Array(Buffer.from('Hello Node.js'));
fs.writeFile('message.txt', data, { signal }, (err) => {
// When a request is aborted - the callback is called with an AbortError
Copy link
Member

Choose a reason for hiding this comment

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

This may be a bit difficult, and is likely something that we should tackle later in a separate PR, but it would be helpful if this reported the number of bytes known to have been written at the point of failure/cancelation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I asked in the description it's actually pretty easy to do - but I remember there were really good arguments regarding whether or not we should do this (in the summit, by @addaleax IIRC) and I don't remember what the conclusion was.

Adding the number of bytes written is actually technically pretty easy here if we just want to approximate how much was written since the last write since we don't actually abort writes in progress only writeAll

Copy link
Member

Choose a reason for hiding this comment

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

Right – I think we basically agreed to see cancellation is a way of expressing disinterest in the result of an operation. Not reporting the number of written bytes goes in line with that. More importantly, if we add cancellation support to the actual fs operations underneath – as we should – then these numbers become inherently unreliable. I would not be in favour of adding this, if somebody wants it, then they should use fs.write() directly without cancelling.

});
// When the request should be aborted
controller.abort();
```

Aborting an ongoing request does not abort individual operating
system requests but rather the internal buffering `fs.writeFile` performs.
Copy link
Member

Choose a reason for hiding this comment

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

Users who are unfamiliar with how writeFile works under the covers may find this sentence confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you propose we deal with this? Add an extra note to writeFile explaining how it works and suggesting a createWriteStream for performacne sensitive stuff like we do with readFile?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to explain how this works underneath instead, and then include the above sentence.

Copy link
Member Author

@benjamingr benjamingr Nov 9, 2020

Choose a reason for hiding this comment

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

@mcollina @jasnell done PTAL


### Using `fs.writeFile()` with file descriptors

When `file` is a file descriptor, the behavior is almost identical to directly
Expand Down Expand Up @@ -5717,6 +5744,10 @@ The `atime` and `mtime` arguments follow these rules:
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/35993
description: The options argument may include an AbortSignal to abort an
ongoing writeFile request.
- version: v14.12.0
pr-url: https://github.com/nodejs/node/pull/34993
description: The `data` parameter will stringify an object with an
Expand All @@ -5733,6 +5764,7 @@ changes:
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
* `signal` {AbortSignal} allows aborting an in-progress writeFile
* Returns: {Promise}

Asynchronously writes data to a file, replacing the file if it already exists.
Expand All @@ -5746,7 +5778,34 @@ If `options` is a string, then it specifies the encoding.
Any specified `FileHandle` has to support writing.

It is unsafe to use `fsPromises.writeFile()` multiple times on the same file
without waiting for the `Promise` to be resolved (or rejected).
without waiting for the `Promise` to be fulfilled (or rejected).

Similarly to `fsPromises.readFile` - `fsPromises.writeFile` is a convenience
method that performs multiple `write` calls internally to write the buffer
passed to it. For performance sensitive code consider using
[`fs.createWriteStream()`][].

It is possible to use an {AbortSignal} to cancel an `fsPromises.writeFile()`.
Cancelation is "best effort", and some amount of data is likely still
to be written.

```js
const controller = new AbortController();
const { signal } = controller;
const data = new Uint8Array(Buffer.from('Hello Node.js'));
(async () => {
try {
await fs.writeFile('message.txt', data, { signal });
} catch (err) {
// When a request is aborted - err is an AbortError
}
})();
// When the request should be aborted
controller.abort();
```

Aborting an ongoing request does not abort individual operating
system requests but rather the internal buffering `fs.writeFile` performs.

## FS constants

Expand Down
26 changes: 22 additions & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const {
ERR_INVALID_CALLBACK,
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM
},
hideStackFrames,
uvException
} = require('internal/errors');

Expand Down Expand Up @@ -133,6 +134,13 @@ let ReadStream;
let WriteStream;
let rimraf;
let rimrafSync;
let DOMException;

const lazyDOMException = hideStackFrames((message, name) => {
if (DOMException === undefined)
DOMException = internalBinding('messaging').DOMException;
return new DOMException(message, name);
});

// These have to be separate because of how graceful-fs happens to do it's
// monkeypatching.
Expand Down Expand Up @@ -1409,7 +1417,11 @@ function lutimesSync(path, atime, mtime) {
handleErrorFromBinding(ctx);
}

function writeAll(fd, isUserFd, buffer, offset, length, callback) {
function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
if (signal?.aborted) {
callback(lazyDOMException('The operation was aborted', 'AbortError'));
return;
}
// write(fd, buffer, offset, length, position, callback)
fs.write(fd, buffer, offset, length, null, (writeErr, written) => {
if (writeErr) {
Expand All @@ -1429,7 +1441,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, callback) {
} else {
offset += written;
length -= written;
writeAll(fd, isUserFd, buffer, offset, length, callback);
writeAll(fd, isUserFd, buffer, offset, length, signal, callback);
}
});
}
Expand All @@ -1446,16 +1458,22 @@ function writeFile(path, data, options, callback) {

if (isFd(path)) {
const isUserFd = true;
writeAll(path, isUserFd, data, 0, data.byteLength, callback);
const signal = options.signal;
writeAll(path, isUserFd, data, 0, data.byteLength, signal, callback);
return;
}

if (options.signal?.aborted) {
callback(lazyDOMException('The operation was aborted', 'AbortError'));
return;
}
fs.open(path, flag, options.mode, (openErr, fd) => {
if (openErr) {
callback(openErr);
} else {
const isUserFd = false;
writeAll(fd, isUserFd, data, 0, data.byteLength, callback);
const signal = options.signal;
writeAll(fd, isUserFd, data, 0, data.byteLength, signal, callback);
}
});
}
Expand Down
10 changes: 8 additions & 2 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,15 @@ async function fsCall(fn, handle, ...args) {
}
}

async function writeFileHandle(filehandle, data) {
async function writeFileHandle(filehandle, data, signal) {
// `data` could be any kind of typed array.
data = new Uint8Array(data.buffer, data.byteOffset, data.byteLength);
let remaining = data.length;
if (remaining === 0) return;
do {
if (signal?.aborted) {
throw new lazyDOMException('The operation was aborted', 'AbortError');
}
const { bytesWritten } =
await write(filehandle, data, 0,
MathMin(kWriteFileMaxChunkSize, data.length));
Expand Down Expand Up @@ -633,9 +636,12 @@ async function writeFile(path, data, options) {
}

if (path instanceof FileHandle)
return writeFileHandle(path, data);
return writeFileHandle(path, data, options.signal);

const fd = await open(path, flag, options.mode);
if (options.signal?.aborted) {
throw new lazyDOMException('The operation was aborted', 'AbortError');
}
return PromisePrototypeFinally(writeFileHandle(fd, data), fd.close);
}

Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-fs-promises-writefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const tmpDir = tmpdir.path;
tmpdir.refresh();

const dest = path.resolve(tmpDir, 'tmp.txt');
const otherDest = path.resolve(tmpDir, 'tmp-2.txt');
const buffer = Buffer.from('abc'.repeat(1000));
const buffer2 = Buffer.from('xyz'.repeat(1000));

Expand All @@ -20,6 +21,15 @@ async function doWrite() {
assert.deepStrictEqual(data, buffer);
}

async function doWriteWithCancel() {
const controller = new AbortController();
const { signal } = controller;
process.nextTick(() => controller.abort());
assert.rejects(fsPromises.writeFile(otherDest, buffer, { signal }), {
name: 'AbortError'
});
}

async function doAppend() {
await fsPromises.appendFile(dest, buffer2);
const data = fs.readFileSync(dest);
Expand All @@ -41,6 +51,7 @@ async function doReadWithEncoding() {
}

doWrite()
.then(doWriteWithCancel)
.then(doAppend)
.then(doRead)
.then(doReadWithEncoding)
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-fs-write-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,32 @@ fs.open(filename4, 'w+', common.mustSucceed((fd) => {
}));
}));
}));


{
// Test that writeFile is cancellable with an AbortSignal.
// Before the operation has started
const controller = new AbortController();
const signal = controller.signal;
const filename3 = join(tmpdir.path, 'test3.txt');

fs.writeFile(filename3, s, { signal }, common.mustCall((err) => {
assert.strictEqual(err.name, 'AbortError');
}));

controller.abort();
}

{
// Test that writeFile is cancellable with an AbortSignal.
// After the operation has started
const controller = new AbortController();
const signal = controller.signal;
const filename4 = join(tmpdir.path, 'test4.txt');

fs.writeFile(filename4, s, { signal }, common.mustCall((err) => {
assert.strictEqual(err.name, 'AbortError');
}));

process.nextTick(() => controller.abort());
}