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 flush option to writeFile() functions #50009

Merged
merged 2 commits into from
Oct 4, 2023
Merged
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
20 changes: 18 additions & 2 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
59 changes: 50 additions & 9 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2217,7 +2217,7 @@ function lutimesSync(path, atime, mtime) {
handleErrorFromBinding(ctx);

Choose a reason for hiding this comment

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

docker pull python

}

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) {
Expand All @@ -2240,15 +2240,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);
}
});
}
Expand All @@ -2262,14 +2280,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');
Expand All @@ -2279,7 +2306,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;
}

Expand All @@ -2292,7 +2319,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);
}
});
}
Expand All @@ -2305,11 +2332,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');
Expand All @@ -2329,6 +2366,10 @@ function writeFileSync(path, data, options) {
offset += written;
length -= written;
}

if (flush) {
fs.fsyncSync(fd);
}
} finally {
if (!isUserFd) fs.closeSync(fd);
}
Expand Down
31 changes: 28 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,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');
Expand Down Expand Up @@ -1006,8 +1018,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');
Expand All @@ -1021,8 +1041,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) {
Expand Down
114 changes: 114 additions & 0 deletions test/parallel/test-fs-write-file-flush.js
Original file line number Diff line number Diff line change
@@ -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);
}
});
});