From 92bdfd141b5d147c0b22e6d495b59c1b119f656b Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Sun, 1 Nov 2020 18:22:20 +0200 Subject: [PATCH] fs: add support for AbortSignal in readFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/35911 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: Michaël Zasso --- doc/api/fs.md | 42 +++++++++++++++++++ lib/fs.js | 3 ++ lib/internal/fs/promises.js | 25 ++++++++++- lib/internal/fs/read_file_context.js | 16 +++++++ lib/internal/fs/utils.js | 5 +++ test/parallel/test-fs-promises-readfile.js | 49 ++++++++++++++++------ test/parallel/test-fs-readfile.js | 18 ++++++++ 7 files changed, 144 insertions(+), 14 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 24768ac17f97e3..e20ab0f1fd9c1b 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -3031,6 +3031,10 @@ If `options.withFileTypes` is set to `true`, the result will contain * `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. @@ -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])` diff --git a/lib/fs.js b/lib/fs.js index abd12e131904c8..fa64ae7e53d79c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -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); diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 497605aaefa72a..39c662fdc5b9c8 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -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 { @@ -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); +}); + class FileHandle extends JSTransferable { constructor(filehandle) { super(); @@ -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 */]; @@ -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); diff --git a/lib/internal/fs/read_file_context.js b/lib/internal/fs/read_file_context.js index 5091a34231665b..faca0e0c331e39 100644 --- a/lib/internal/fs/read_file_context.js +++ b/lib/internal/fs/read_file_context.js @@ -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 @@ -74,6 +84,7 @@ class ReadFileContext { this.pos = 0; this.encoding = encoding; this.err = null; + this.signal = undefined; } read() { @@ -81,6 +92,11 @@ class ReadFileContext { 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; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index a68a9a32b9cc4a..24e2224c2e3945 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -35,6 +35,7 @@ const { const { once } = require('internal/util'); const { toPathIfFileURL } = require('internal/url'); const { + validateAbortSignal, validateBoolean, validateInt32, validateUint32 @@ -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; } diff --git a/test/parallel/test-fs-promises-readfile.js b/test/parallel/test-fs-promises-readfile.js index ff25be75b84ff0..93729153ff3019 100644 --- a/test/parallel/test-fs-promises-readfile.js +++ b/test/parallel/test-fs-promises-readfile.js @@ -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() { @@ -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()); diff --git a/test/parallel/test-fs-readfile.js b/test/parallel/test-fs-readfile.js index 648bf692d1dcc8..751043886f0d96 100644 --- a/test/parallel/test-fs-readfile.js +++ b/test/parallel/test-fs-readfile.js @@ -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()); +}