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: add support for AbortSignal in readFile #35911

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
42 changes: 42 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -3031,6 +3031,10 @@ If `options.withFileTypes` is set to `true`, the result will contain
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https:/nodejs/node/pull/35911
description: The options argument may include an AbortSignal to abort an
ongoing readFile request.
- version: v10.0.0
pr-url: https:/nodejs/node/pull/12562
description: The `callback` parameter is no longer optional. Not passing
Expand All @@ -3056,6 +3060,7 @@ changes:
* `options` {Object|string}
* `encoding` {string|null} **Default:** `null`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'r'`.
* `signal` {AbortSignal} allows aborting an in-progress readFile
* `callback` {Function}
* `err` {Error}
* `data` {string|Buffer}
Expand Down Expand Up @@ -3097,9 +3102,25 @@ fs.readFile('<directory>', (err, data) => {
});
```

It is possible to abort an ongoing request using an `AbortSignal`. If a
request is aborted the callback is called with an `AbortError`:

```js
const controller = new AbortController();
const signal = controller.signal;
fs.readFile(fileInfo[0].name, { signal }, (err, buf) => {
// ...
});
// When you want to abort the request
controller.abort();
```

The `fs.readFile()` function buffers the entire file. To minimize memory costs,
when possible prefer streaming via `fs.createReadStream()`.

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

### File descriptors

1. Any specified file descriptor has to support reading.
Expand Down Expand Up @@ -4771,6 +4792,7 @@ added: v10.0.0

* `options` {Object|string}
* `encoding` {string|null} **Default:** `null`
* `signal` {AbortSignal} allows aborting an in-progress readFile
* Returns: {Promise}

Asynchronously reads the entire contents of a file.
Expand Down Expand Up @@ -5438,12 +5460,18 @@ print('./').catch(console.error);
### `fsPromises.readFile(path[, options])`
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https:/nodejs/node/pull/35911
description: The options argument may include an AbortSignal to abort an
ongoing readFile request.
-->

* `path` {string|Buffer|URL|FileHandle} filename or `FileHandle`
* `options` {Object|string}
* `encoding` {string|null} **Default:** `null`
* `flag` {string} See [support of file system `flags`][]. **Default:** `'r'`.
* `signal` {AbortSignal} allows aborting an in-progress readFile
* Returns: {Promise}

Asynchronously reads the entire contents of a file.
Expand All @@ -5459,6 +5487,20 @@ platform-specific. On macOS, Linux, and Windows, the promise will be rejected
with an error. On FreeBSD, a representation of the directory's contents will be
returned.

It is possible to abort an ongoing `readFile` using an `AbortSignal`. If a
request is aborted the promise returned is rejected with an `AbortError`:

```js
const controller = new AbortController();
const signal = controller.signal;
readFile(fileName, { signal }).then((file) => { /* ... */ });
// Abort the request
controller.abort();
```

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

Any specified `FileHandle` has to support reading.

### `fsPromises.readlink(path[, options])`
Expand Down
3 changes: 3 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,9 @@ function readFile(path, options, callback) {
const context = new ReadFileContext(callback, options.encoding);
context.isUserFd = isFd(path); // File descriptor ownership

if (options.signal) {
context.signal = options.signal;
}
if (context.isUserFd) {
process.nextTick(function tick(context) {
readFileAfterOpen.call({ context }, null, path);
Expand Down
25 changes: 23 additions & 2 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@ const {
} = internalBinding('constants').fs;
const binding = internalBinding('fs');
const { Buffer } = require('buffer');

const { codes, hideStackFrames } = require('internal/errors');
const {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_METHOD_NOT_IMPLEMENTED
} = require('internal/errors').codes;
ERR_METHOD_NOT_IMPLEMENTED,
} = codes;
const { isArrayBufferView } = require('internal/util/types');
const { rimrafPromises } = require('internal/fs/rimraf');
const {
Expand Down Expand Up @@ -82,6 +84,13 @@ const {
const getDirectoryEntriesPromise = promisify(getDirents);
const validateRmOptionsPromise = promisify(validateRmOptions);

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

Choose a reason for hiding this comment

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

Why using a DOMException? As a user I would prefer to receive one of our error objects with a code.

Copy link
Member

Choose a reason for hiding this comment

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

This is recommended by the spec: https://dom.spec.whatwg.org/#aborting-ongoing-activities

Users are supposed to check for it with error.name === 'AbortError'

Copy link
Member

Choose a reason for hiding this comment

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

We are telling our users to check for an error code. I think that should be the way to go here as well.

Note that I'd like the error code to be specific for fs.readFile, so it's easy to debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina this is what we use everywhere else that uses AbortSignals (like events.once and timers/promises) and everywhere in the web standard (like fetch). I feel strongly that this should be compatible with how exceptions work everywhere else.

A code is already set on DOMExceptions (as ABORT_ERR ) see internal/per_context/domexception - we can recommend the code as the way to check rather than the name in the docs if you think that's better.

Copy link
Member

Choose a reason for hiding this comment

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

I've likely missed it everywhere else. This seems really hard to debug: users will not know what this aborterror is about in a complex application.

Let's not hold this for it.. I'll open an issue when I have the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've likely missed it everywhere else. This seems really hard to debug: users will not know what this aborterror is about in a complex application.

Why? Would it help if this had a different .code? I am happy to set the code to something different here and reject with a subclass if you think that improves DX. I can also do that for timers (though like you said probably better in a different PX).

Note I'm not convinced DX is actually better with "different aborts cancel with different codes since by far the most common the use case I saw is checking for cancellation (in general) to ignore the error (since cancellation is not an error most times). Users can also figure where cancellation came from from the stack trace.

Let's not hold this for it.. I'll open an issue when I have the time.

Sure, note you are currently blocking - is that intentional?


class FileHandle extends JSTransferable {
constructor(filehandle) {
super();
Expand Down Expand Up @@ -259,8 +268,17 @@ async function writeFileHandle(filehandle, data) {
}

async function readFileHandle(filehandle, options) {
const signal = options && options.signal;

if (signal && signal.aborted) {
throw lazyDOMException('The operation was aborted', 'AbortError');
}
const statFields = await binding.fstat(filehandle.fd, false, kUsePromises);

if (signal && signal.aborted) {
throw lazyDOMException('The operation was aborted', 'AbortError');
}

let size;
if ((statFields[1/* mode */] & S_IFMT) === S_IFREG) {
size = statFields[8/* size */];
Expand All @@ -277,6 +295,9 @@ async function readFileHandle(filehandle, options) {
MathMin(size, kReadFileMaxChunkSize);
let endOfFile = false;
do {
if (signal && signal.aborted) {
throw lazyDOMException('The operation was aborted', 'AbortError');
}
const buf = Buffer.alloc(chunkSize);
const { bytesRead, buffer } =
await read(filehandle, buf, 0, chunkSize, -1);
Expand Down
16 changes: 16 additions & 0 deletions lib/internal/fs/read_file_context.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ const { Buffer } = require('buffer');

const { FSReqCallback, close, read } = internalBinding('fs');

const { hideStackFrames } = require('internal/errors');


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

// Use 64kb in case the file type is not a regular file and thus do not know the
// actual file size. Increasing the value further results in more frequent over
// allocation for small files and consumes CPU time and memory that should be
Expand Down Expand Up @@ -74,13 +84,19 @@ class ReadFileContext {
this.pos = 0;
this.encoding = encoding;
this.err = null;
this.signal = undefined;
}

read() {
let buffer;
let offset;
let length;

if (this.signal && this.signal.aborted) {
return this.close(
lazyDOMException('The operation was aborted', 'AbortError')
);
}
if (this.size === 0) {
buffer = Buffer.allocUnsafeSlow(kReadFileUnknownBufferLength);
offset = 0;
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const {
const { once } = require('internal/util');
const { toPathIfFileURL } = require('internal/url');
const {
validateAbortSignal,
validateBoolean,
validateInt32,
validateUint32
Expand Down Expand Up @@ -296,6 +297,10 @@ function getOptions(options, defaultOptions) {

if (options.encoding !== 'buffer')
assertEncoding(options.encoding);

if (options.signal !== undefined) {
validateAbortSignal(options.signal, 'options.signal');
}
return options;
}

Expand Down
49 changes: 37 additions & 12 deletions test/parallel/test-fs-promises-readfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,21 @@ tmpdir.refresh();

const fn = path.join(tmpdir.path, 'large-file');

async function validateReadFile() {
// Creating large buffer with random content
const buffer = Buffer.from(
Array.apply(null, { length: 16834 * 2 })
.map(Math.random)
.map((number) => (number * (1 << 8)))
);
// Creating large buffer with random content
const largeBuffer = Buffer.from(
Array.apply(null, { length: 16834 * 2 })
.map(Math.random)
.map((number) => (number * (1 << 8)))
);

async function createLargeFile() {
// Writing buffer to a file then try to read it
await writeFile(fn, buffer);
await writeFile(fn, largeBuffer);
}

async function validateReadFile() {
const readBuffer = await readFile(fn);
assert.strictEqual(readBuffer.equals(buffer), true);
assert.strictEqual(readBuffer.equals(largeBuffer), true);
}

async function validateReadFileProc() {
Expand All @@ -39,6 +42,28 @@ async function validateReadFileProc() {
assert.ok(hostname.length > 0);
}

validateReadFile()
.then(() => validateReadFileProc())
.then(common.mustCall());
function validateReadFileAbortLogicBefore() {
const controller = new AbortController();
const signal = controller.signal;
controller.abort();
assert.rejects(readFile(fn, { signal }), {
name: 'AbortError'
});
}

function validateReadFileAbortLogicDuring() {
const controller = new AbortController();
const signal = controller.signal;
process.nextTick(() => controller.abort());
assert.rejects(readFile(fn, { signal }), {
name: 'AbortError'
});
}

(async () => {
await createLargeFile();
await validateReadFile();
await validateReadFileProc();
await validateReadFileAbortLogicBefore();
await validateReadFileAbortLogicDuring();
})().then(common.mustCall());
18 changes: 18 additions & 0 deletions test/parallel/test-fs-readfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,21 @@ for (const e of fileInfo) {
assert.deepStrictEqual(buf, e.contents);
}));
}
{
// Test cancellation, before
const controller = new AbortController();
const signal = controller.signal;
controller.abort();
fs.readFile(fileInfo[0].name, { signal }, common.mustCall((err, buf) => {
assert.strictEqual(err.name, 'AbortError');
}));
}
{
// Test cancellation, during read
const controller = new AbortController();
const signal = controller.signal;
fs.readFile(fileInfo[0].name, { signal }, common.mustCall((err, buf) => {
assert.strictEqual(err.name, 'AbortError');
}));
process.nextTick(() => controller.abort());
}