From 39b8c28d0752830d66507085d3b6f9223a4cc15c Mon Sep 17 00:00:00 2001
From: Benjamin Gruenbaum <benjamingr@gmail.com>
Date: Fri, 6 Nov 2020 13:44:40 +0200
Subject: [PATCH] fs: support abortsignal in writeFile

PR-URL: https://github.com/nodejs/node/pull/35993
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 doc/api/fs.md                               | 61 ++++++++++++++++++++-
 lib/fs.js                                   | 26 +++++++--
 lib/internal/fs/promises.js                 | 10 +++-
 test/parallel/test-fs-promises-writefile.js | 12 ++++
 test/parallel/test-fs-write-file.js         | 30 ++++++++++
 5 files changed, 132 insertions(+), 7 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 5c764e50cc3a22..5ea61f70c95c4d 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -4361,6 +4361,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
@@ -4395,6 +4399,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}
 
@@ -4426,6 +4431,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
+});
+// 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.
+
 ### Using `fs.writeFile()` with file descriptors
 
 When `file` is a file descriptor, the behavior is almost identical to directly
@@ -5670,6 +5697,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
@@ -5686,6 +5717,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.
@@ -5699,7 +5731,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
 
diff --git a/lib/fs.js b/lib/fs.js
index 24a4cc9b159ea2..c794d8f678be5b 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -71,6 +71,7 @@ const {
     ERR_INVALID_CALLBACK,
     ERR_FEATURE_UNAVAILABLE_ON_PLATFORM
   },
+  hideStackFrames,
   uvException
 } = require('internal/errors');
 
@@ -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.
@@ -1405,7 +1413,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) {
@@ -1425,7 +1437,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);
     }
   });
 }
@@ -1442,16 +1454,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);
     }
   });
 }
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index d4e83947feb193..a6b50054774f4e 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -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));
@@ -644,9 +647,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);
 }
 
diff --git a/test/parallel/test-fs-promises-writefile.js b/test/parallel/test-fs-promises-writefile.js
index 858e90bc6291de..de58395774ba0d 100644
--- a/test/parallel/test-fs-promises-writefile.js
+++ b/test/parallel/test-fs-promises-writefile.js
@@ -1,3 +1,4 @@
+// Flags: --experimental-abortcontroller
 'use strict';
 
 const common = require('../common');
@@ -11,6 +12,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));
 
@@ -20,6 +22,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);
@@ -41,6 +52,7 @@ async function doReadWithEncoding() {
 }
 
 doWrite()
+  .then(doWriteWithCancel)
   .then(doAppend)
   .then(doRead)
   .then(doReadWithEncoding)
diff --git a/test/parallel/test-fs-write-file.js b/test/parallel/test-fs-write-file.js
index e80861756ab2d8..3484782a8d4071 100644
--- a/test/parallel/test-fs-write-file.js
+++ b/test/parallel/test-fs-write-file.js
@@ -19,6 +19,7 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
+// Flags: --experimental-abortcontroller
 'use strict';
 const common = require('../common');
 const assert = require('assert');
@@ -66,3 +67,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());
+}