From 8f4b924f4a7b37bd16ddff65329c8e96fc5f0f2d Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Wed, 17 Oct 2018 14:40:08 +0530 Subject: [PATCH] fs: make writeFile consistent with readFile wrt fd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As it is, `readFile` always reads from the current position of the file, if a file descriptor is used. But `writeFile` always writes from the beginning of the file. This patch fixes this inconsistency by making `writeFile` also to write from the current position of the file when used with a file descriptor. PR-URL: https://github.com/nodejs/node/pull/23709 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Vse Mozhet Byt Reviewed-By: Sam Roberts Reviewed-By: Matteo Collina Reviewed-By: Daniel Bevenius Reviewed-By: Rod Vagg Reviewed-By: Anatoli Papirovski Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Ruben Bridgewater --- lib/fs.js | 4 +- .../test-fs-promises-readfile-with-fd.js | 35 +++++++++++ .../test-fs-promises-writefile-with-fd.js | 36 ++++++++++++ test/parallel/test-fs-readfile-fd.js | 50 +++++++++++++++- test/parallel/test-fs-writefile-with-fd.js | 58 +++++++++++++++++++ 5 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-fs-promises-readfile-with-fd.js create mode 100644 test/parallel/test-fs-promises-writefile-with-fd.js create mode 100644 test/parallel/test-fs-writefile-with-fd.js diff --git a/lib/fs.js b/lib/fs.js index 335b1644838f07..873e93d9cde828 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1229,7 +1229,7 @@ function writeFile(path, data, options, callback) { function writeFd(fd, isUserFd) { const buffer = isArrayBufferView(data) ? data : Buffer.from('' + data, options.encoding || 'utf8'); - const position = /a/.test(flag) ? null : 0; + const position = (/a/.test(flag) || isUserFd) ? null : 0; writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback); } @@ -1247,7 +1247,7 @@ function writeFileSync(path, data, options) { } let offset = 0; let length = data.byteLength; - let position = /a/.test(flag) ? null : 0; + let position = (/a/.test(flag) || isUserFd) ? null : 0; try { while (length > 0) { const written = fs.writeSync(fd, data, offset, length, position); diff --git a/test/parallel/test-fs-promises-readfile-with-fd.js b/test/parallel/test-fs-promises-readfile-with-fd.js new file mode 100644 index 00000000000000..9bf4c185364949 --- /dev/null +++ b/test/parallel/test-fs-promises-readfile-with-fd.js @@ -0,0 +1,35 @@ +'use strict'; + +/* + * This test makes sure that `readFile()` always reads from the current + * position of the file, instead of reading from the beginning of the file. + */ + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const { writeFileSync } = require('fs'); +const { open } = require('fs').promises; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const fn = path.join(tmpdir.path, 'test.txt'); +writeFileSync(fn, 'Hello World'); + +async function readFileTest() { + const handle = await open(fn, 'r'); + + /* Read only five bytes, so that the position moves to five. */ + const buf = Buffer.alloc(5); + const { bytesRead } = await handle.read(buf, 0, 5, null); + assert.strictEqual(bytesRead, 5); + assert.deepStrictEqual(buf.toString(), 'Hello'); + + /* readFile() should read from position five, instead of zero. */ + assert.deepStrictEqual((await handle.readFile()).toString(), ' World'); +} + + +readFileTest() + .then(common.mustCall()); diff --git a/test/parallel/test-fs-promises-writefile-with-fd.js b/test/parallel/test-fs-promises-writefile-with-fd.js new file mode 100644 index 00000000000000..a4cc31df7f4d5f --- /dev/null +++ b/test/parallel/test-fs-promises-writefile-with-fd.js @@ -0,0 +1,36 @@ +'use strict'; + +/* + * This test makes sure that `writeFile()` always writes from the current + * position of the file, instead of truncating the file. + */ + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const { readFileSync } = require('fs'); +const { open } = require('fs').promises; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const fn = path.join(tmpdir.path, 'test.txt'); + +async function writeFileTest() { + const handle = await open(fn, 'w'); + + /* Write only five bytes, so that the position moves to five. */ + const buf = Buffer.from('Hello'); + const { bytesWritten } = await handle.write(buf, 0, 5, null); + assert.strictEqual(bytesWritten, 5); + + /* Write some more with writeFile(). */ + await handle.writeFile('World'); + + /* New content should be written at position five, instead of zero. */ + assert.deepStrictEqual(readFileSync(fn).toString(), 'HelloWorld'); +} + + +writeFileTest() + .then(common.mustCall()); diff --git a/test/parallel/test-fs-readfile-fd.js b/test/parallel/test-fs-readfile-fd.js index b5d2a33e285f5f..df0f428bf153c0 100644 --- a/test/parallel/test-fs-readfile-fd.js +++ b/test/parallel/test-fs-readfile-fd.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); // Test fs.readFile using a file descriptor. @@ -7,6 +7,9 @@ const fixtures = require('../common/fixtures'); const assert = require('assert'); const fs = require('fs'); const fn = fixtures.path('empty.txt'); +const join = require('path').join; +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); tempFd(function(fd, close) { fs.readFile(fd, function(err, data) { @@ -46,3 +49,48 @@ function tempFdSync(callback) { callback(fd); fs.closeSync(fd); } + +{ + /* + * This test makes sure that `readFile()` always reads from the current + * position of the file, instead of reading from the beginning of the file, + * when used with file descriptors. + */ + + const filename = join(tmpdir.path, 'test.txt'); + fs.writeFileSync(filename, 'Hello World'); + + { + /* Tests the fs.readFileSync(). */ + const fd = fs.openSync(filename, 'r'); + + /* Read only five bytes, so that the position moves to five. */ + const buf = Buffer.alloc(5); + assert.deepStrictEqual(fs.readSync(fd, buf, 0, 5), 5); + assert.deepStrictEqual(buf.toString(), 'Hello'); + + /* readFileSync() should read from position five, instead of zero. */ + assert.deepStrictEqual(fs.readFileSync(fd).toString(), ' World'); + } + + { + /* Tests the fs.readFile(). */ + fs.open(filename, 'r', common.mustCall((err, fd) => { + assert.ifError(err); + const buf = Buffer.alloc(5); + + /* Read only five bytes, so that the position moves to five. */ + fs.read(fd, buf, 0, 5, null, common.mustCall((err, bytes) => { + assert.ifError(err); + assert.strictEqual(bytes, 5); + assert.deepStrictEqual(buf.toString(), 'Hello'); + + fs.readFile(fd, common.mustCall((err, data) => { + assert.ifError(err); + /* readFile() should read from position five, instead of zero. */ + assert.deepStrictEqual(data.toString(), ' World'); + })); + })); + })); + } +} diff --git a/test/parallel/test-fs-writefile-with-fd.js b/test/parallel/test-fs-writefile-with-fd.js new file mode 100644 index 00000000000000..6851518d4dcc9a --- /dev/null +++ b/test/parallel/test-fs-writefile-with-fd.js @@ -0,0 +1,58 @@ +'use strict'; + +/* + * This test makes sure that `writeFile()` always writes from the current + * position of the file, instead of truncating the file, when used with file + * descriptors. + */ + +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs'); +const join = require('path').join; + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +{ + /* writeFileSync() test. */ + const filename = join(tmpdir.path, 'test.txt'); + + /* Open the file descriptor. */ + const fd = fs.openSync(filename, 'w'); + + /* Write only five characters, so that the position moves to five. */ + assert.deepStrictEqual(fs.writeSync(fd, 'Hello'), 5); + assert.deepStrictEqual(fs.readFileSync(filename).toString(), 'Hello'); + + /* Write some more with writeFileSync(). */ + fs.writeFileSync(fd, 'World'); + + /* New content should be written at position five, instead of zero. */ + assert.deepStrictEqual(fs.readFileSync(filename).toString(), 'HelloWorld'); +} + +{ + /* writeFile() test. */ + const file = join(tmpdir.path, 'test1.txt'); + + /* Open the file descriptor. */ + fs.open(file, 'w', common.mustCall((err, fd) => { + assert.ifError(err); + + /* Write only five characters, so that the position moves to five. */ + fs.write(fd, 'Hello', common.mustCall((err, bytes) => { + assert.ifError(err); + assert.strictEqual(bytes, 5); + assert.deepStrictEqual(fs.readFileSync(file).toString(), 'Hello'); + + /* Write some more with writeFile(). */ + fs.writeFile(fd, 'World', common.mustCall((err) => { + assert.ifError(err); + + /* New content should be written at position five, instead of zero. */ + assert.deepStrictEqual(fs.readFileSync(file).toString(), 'HelloWorld'); + })); + })); + })); +}