From e01c1d700d9fdb197ea40a444ebd228337a7dd6e Mon Sep 17 00:00:00 2001
From: Colin Ihrig <cjihrig@gmail.com>
Date: Wed, 4 Oct 2023 09:10:30 -0400
Subject: [PATCH] fs: add flush option to writeFile() functions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This commit adds a 'flush' option to the fs.writeFile family of
functions.

Refs: https://github.com/nodejs/node/issues/49886
PR-URL: https://github.com/nodejs/node/pull/50009
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
---
 doc/api/fs.md                             |  20 +++-
 lib/fs.js                                 |  59 +++++++++--
 lib/internal/fs/promises.js               |  31 +++++-
 test/parallel/test-fs-write-file-flush.js | 114 ++++++++++++++++++++++
 4 files changed, 210 insertions(+), 14 deletions(-)
 create mode 100644 test/parallel/test-fs-write-file-flush.js

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 34d10418b2acc8..76f6432212654f 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -1742,6 +1742,9 @@ All the [caveats][] for `fs.watch()` also apply to `fsPromises.watch()`.
 <!-- YAML
 added: v10.0.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/50009
+    description: The `flush` option is now supported.
   - version:
       - v15.14.0
       - v14.18.0
@@ -1765,6 +1768,9 @@ changes:
   * `encoding` {string|null} **Default:** `'utf8'`
   * `mode` {integer} **Default:** `0o666`
   * `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
+  * `flush` {boolean} If all data is successfully written to the file, and
+    `flush` is `true`, `filehandle.sync()` is used to flush the data.
+    **Default:** `false`.
   * `signal` {AbortSignal} allows aborting an in-progress writeFile
 * Returns: {Promise} Fulfills with `undefined` upon success.
 
@@ -4879,6 +4885,9 @@ details.
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/50009
+    description: The `flush` option is now supported.
   - version: v19.0.0
     pr-url: https://github.com/nodejs/node/pull/42796
     description: Passing to the `string` parameter an object with an own
@@ -4936,6 +4945,9 @@ changes:
   * `encoding` {string|null} **Default:** `'utf8'`
   * `mode` {integer} **Default:** `0o666`
   * `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
+  * `flush` {boolean} If all data is successfully written to the file, and
+    `flush` is `true`, `fs.fsync()` is used to flush the data.
+    **Default:** `false`.
   * `signal` {AbortSignal} allows aborting an in-progress writeFile
 * `callback` {Function}
   * `err` {Error|AggregateError}
@@ -6167,6 +6179,9 @@ this API: [`fs.utimes()`][].
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/50009
+    description: The `flush` option is now supported.
   - version: v19.0.0
     pr-url: https://github.com/nodejs/node/pull/42796
     description: Passing to the `data` parameter an object with an own
@@ -6201,8 +6216,9 @@ changes:
   * `encoding` {string|null} **Default:** `'utf8'`
   * `mode` {integer} **Default:** `0o666`
   * `flag` {string} See [support of file system `flags`][]. **Default:** `'w'`.
-
-Returns `undefined`.
+  * `flush` {boolean} If all data is successfully written to the file, and
+    `flush` is `true`, `fs.fsyncSync()` is used to flush the data.
+    Returns `undefined`.
 
 The `mode` option only affects the newly created file. See [`fs.open()`][]
 for more details.
diff --git a/lib/fs.js b/lib/fs.js
index 585458fddd6c1f..684ac76a12ce50 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -2216,7 +2216,7 @@ function lutimesSync(path, atime, mtime) {
   handleErrorFromBinding(ctx);
 }
 
-function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
+function writeAll(fd, isUserFd, buffer, offset, length, signal, flush, callback) {
   if (signal?.aborted) {
     const abortError = new AbortError(undefined, { cause: signal?.reason });
     if (isUserFd) {
@@ -2239,15 +2239,33 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
         });
       }
     } else if (written === length) {
-      if (isUserFd) {
-        callback(null);
+      if (!flush) {
+        if (isUserFd) {
+          callback(null);
+        } else {
+          fs.close(fd, callback);
+        }
       } else {
-        fs.close(fd, callback);
+        fs.fsync(fd, (syncErr) => {
+          if (syncErr) {
+            if (isUserFd) {
+              callback(syncErr);
+            } else {
+              fs.close(fd, (err) => {
+                callback(aggregateTwoErrors(err, syncErr));
+              });
+            }
+          } else if (isUserFd) {
+            callback(null);
+          } else {
+            fs.close(fd, callback);
+          }
+        });
       }
     } else {
       offset += written;
       length -= written;
-      writeAll(fd, isUserFd, buffer, offset, length, signal, callback);
+      writeAll(fd, isUserFd, buffer, offset, length, signal, flush, callback);
     }
   });
 }
@@ -2261,14 +2279,23 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
  *   mode?: number;
  *   flag?: string;
  *   signal?: AbortSignal;
+ *   flush?: boolean;
  *   } | string} [options]
  * @param {(err?: Error) => any} callback
  * @returns {void}
  */
 function writeFile(path, data, options, callback) {
   callback = maybeCallback(callback || options);
-  options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
+  options = getOptions(options, {
+    encoding: 'utf8',
+    mode: 0o666,
+    flag: 'w',
+    flush: false,
+  });
   const flag = options.flag || 'w';
+  const flush = options.flush ?? false;
+
+  validateBoolean(flush, 'options.flush');
 
   if (!isArrayBufferView(data)) {
     validateStringAfterArrayBufferView(data, 'data');
@@ -2278,7 +2305,7 @@ function writeFile(path, data, options, callback) {
   if (isFd(path)) {
     const isUserFd = true;
     const signal = options.signal;
-    writeAll(path, isUserFd, data, 0, data.byteLength, signal, callback);
+    writeAll(path, isUserFd, data, 0, data.byteLength, signal, flush, callback);
     return;
   }
 
@@ -2291,7 +2318,7 @@ function writeFile(path, data, options, callback) {
     } else {
       const isUserFd = false;
       const signal = options.signal;
-      writeAll(fd, isUserFd, data, 0, data.byteLength, signal, callback);
+      writeAll(fd, isUserFd, data, 0, data.byteLength, signal, flush, callback);
     }
   });
 }
@@ -2304,11 +2331,21 @@ function writeFile(path, data, options, callback) {
  *   encoding?: string | null;
  *   mode?: number;
  *   flag?: string;
+ *   flush?: boolean;
  *   } | string} [options]
  * @returns {void}
  */
 function writeFileSync(path, data, options) {
-  options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
+  options = getOptions(options, {
+    encoding: 'utf8',
+    mode: 0o666,
+    flag: 'w',
+    flush: false,
+  });
+
+  const flush = options.flush ?? false;
+
+  validateBoolean(flush, 'options.flush');
 
   if (!isArrayBufferView(data)) {
     validateStringAfterArrayBufferView(data, 'data');
@@ -2328,6 +2365,10 @@ function writeFileSync(path, data, options) {
       offset += written;
       length -= written;
     }
+
+    if (flush) {
+      fs.fsyncSync(fd);
+    }
   } finally {
     if (!isUserFd) fs.closeSync(fd);
   }
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 28fb0127d71f66..a0384a97e2e6ce 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -414,6 +414,18 @@ async function handleFdClose(fileOpPromise, closeFunc) {
   );
 }
 
+async function handleFdSync(fileOpPromise, handle) {
+  return PromisePrototypeThen(
+    fileOpPromise,
+    (result) => PromisePrototypeThen(
+      handle.sync(),
+      () => result,
+      (syncError) => PromiseReject(syncError),
+    ),
+    (opError) => PromiseReject(opError),
+  );
+}
+
 async function fsCall(fn, handle, ...args) {
   assert(handle[kRefs] !== undefined,
          'handle must be an instance of FileHandle');
@@ -1007,8 +1019,16 @@ async function mkdtemp(prefix, options) {
 }
 
 async function writeFile(path, data, options) {
-  options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
+  options = getOptions(options, {
+    encoding: 'utf8',
+    mode: 0o666,
+    flag: 'w',
+    flush: false,
+  });
   const flag = options.flag || 'w';
+  const flush = options.flush ?? false;
+
+  validateBoolean(flush, 'options.flush');
 
   if (!isArrayBufferView(data) && !isCustomIterable(data)) {
     validateStringAfterArrayBufferView(data, 'data');
@@ -1022,8 +1042,13 @@ async function writeFile(path, data, options) {
   checkAborted(options.signal);
 
   const fd = await open(path, flag, options.mode);
-  return handleFdClose(
-    writeFileHandle(fd, data, options.signal, options.encoding), fd.close);
+  let writeOp = writeFileHandle(fd, data, options.signal, options.encoding);
+
+  if (flush) {
+    writeOp = handleFdSync(writeOp, fd);
+  }
+
+  return handleFdClose(writeOp, fd.close);
 }
 
 function isCustomIterable(obj) {
diff --git a/test/parallel/test-fs-write-file-flush.js b/test/parallel/test-fs-write-file-flush.js
new file mode 100644
index 00000000000000..98a8d637c5fa28
--- /dev/null
+++ b/test/parallel/test-fs-write-file-flush.js
@@ -0,0 +1,114 @@
+'use strict';
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('node:assert');
+const fs = require('node:fs');
+const fsp = require('node:fs/promises');
+const test = require('node:test');
+const data = 'foo';
+let cnt = 0;
+
+function nextFile() {
+  return tmpdir.resolve(`${cnt++}.out`);
+}
+
+tmpdir.refresh();
+
+test('synchronous version', async (t) => {
+  await t.test('validation', (t) => {
+    for (const v of ['true', '', 0, 1, [], {}, Symbol()]) {
+      assert.throws(() => {
+        fs.writeFileSync(nextFile(), data, { flush: v });
+      }, { code: 'ERR_INVALID_ARG_TYPE' });
+    }
+  });
+
+  await t.test('performs flush', (t) => {
+    const spy = t.mock.method(fs, 'fsyncSync');
+    const file = nextFile();
+    fs.writeFileSync(file, data, { flush: true });
+    const calls = spy.mock.calls;
+    assert.strictEqual(calls.length, 1);
+    assert.strictEqual(calls[0].result, undefined);
+    assert.strictEqual(calls[0].error, undefined);
+    assert.strictEqual(calls[0].arguments.length, 1);
+    assert.strictEqual(typeof calls[0].arguments[0], 'number');
+    assert.strictEqual(fs.readFileSync(file, 'utf8'), data);
+  });
+
+  await t.test('does not perform flush', (t) => {
+    const spy = t.mock.method(fs, 'fsyncSync');
+
+    for (const v of [undefined, null, false]) {
+      const file = nextFile();
+      fs.writeFileSync(file, data, { flush: v });
+      assert.strictEqual(fs.readFileSync(file, 'utf8'), data);
+    }
+
+    assert.strictEqual(spy.mock.calls.length, 0);
+  });
+});
+
+test('callback version', async (t) => {
+  await t.test('validation', (t) => {
+    for (const v of ['true', '', 0, 1, [], {}, Symbol()]) {
+      assert.throws(() => {
+        fs.writeFileSync(nextFile(), data, { flush: v });
+      }, { code: 'ERR_INVALID_ARG_TYPE' });
+    }
+  });
+
+  await t.test('performs flush', (t, done) => {
+    const spy = t.mock.method(fs, 'fsync');
+    const file = nextFile();
+    fs.writeFile(file, data, { flush: true }, common.mustSucceed(() => {
+      const calls = spy.mock.calls;
+      assert.strictEqual(calls.length, 1);
+      assert.strictEqual(calls[0].result, undefined);
+      assert.strictEqual(calls[0].error, undefined);
+      assert.strictEqual(calls[0].arguments.length, 2);
+      assert.strictEqual(typeof calls[0].arguments[0], 'number');
+      assert.strictEqual(typeof calls[0].arguments[1], 'function');
+      assert.strictEqual(fs.readFileSync(file, 'utf8'), data);
+      done();
+    }));
+  });
+
+  await t.test('does not perform flush', (t, done) => {
+    const values = [undefined, null, false];
+    const spy = t.mock.method(fs, 'fsync');
+    let cnt = 0;
+
+    for (const v of values) {
+      const file = nextFile();
+
+      fs.writeFile(file, data, { flush: v }, common.mustSucceed(() => {
+        assert.strictEqual(fs.readFileSync(file, 'utf8'), data);
+        cnt++;
+
+        if (cnt === values.length) {
+          assert.strictEqual(spy.mock.calls.length, 0);
+          done();
+        }
+      }));
+    }
+  });
+});
+
+test('promise based version', async (t) => {
+  await t.test('validation', async (t) => {
+    for (const v of ['true', '', 0, 1, [], {}, Symbol()]) {
+      await assert.rejects(() => {
+        return fsp.writeFile(nextFile(), data, { flush: v });
+      }, { code: 'ERR_INVALID_ARG_TYPE' });
+    }
+  });
+
+  await t.test('success path', async (t) => {
+    for (const v of [undefined, null, false, true]) {
+      const file = nextFile();
+      await fsp.writeFile(file, data, { flush: v });
+      assert.strictEqual(await fsp.readFile(file, 'utf8'), data);
+    }
+  });
+});