fs: .writeFile(filehandle, ...) behavior differs from the documented one in all its variants

[vsemozhetbyt]

Currently, the fs.writeFile[Sync]() description states:

Asynchronously writes data to a file, replacing the file if it already exists.

However, this is only true if the first argument is a filename. If it is a file descriptor, the file content is not truncated (as somebody may expect) and a new data is merged from the 0 position into the old data.

1. Compare fs.readFileSync() behavior:

'use strict';

const fs = require('fs');

const fileName = 'test.txt';
fs.writeFileSync(fileName, '123');
fs.writeFileSync(fileName, '0');
console.log(fs.readFileSync(fileName, 'utf8'));
fs.unlinkSync(fileName);

const fd = fs.openSync(fileName, 'w');
fs.writeFileSync(fd, '123');
fs.writeFileSync(fd, '0');
fs.closeSync(fd);
console.log(fs.readFileSync(fileName, 'utf8'));
fs.unlinkSync(fileName);

0
023

2. Compare the same fs.writeFile() behavior:

const fs = require('fs');

const fileName = 'test.txt';

fs.writeFile(fileName, '123', () => {
  fs.writeFile(fileName, '0', () => {
    console.log(fs.readFileSync(fileName, 'utf8'));
    fs.unlinkSync(fileName);

    const fd = fs.openSync(fileName, 'w');

    fs.writeFile(fd, '123', () => {
      fs.writeFile(fd, '0', () => {
        fs.closeSync(fd);
        console.log(fs.readFileSync(fileName, 'utf8'));
        fs.unlinkSync(fileName);
      });
    });
  });
});

0
023

If this is intended behavior, should we make the description more accurate?

[Trott]

@nodejs/fs

[vsemozhetbyt]

It seems the fsPromises API is even more erroneous: the data is not merged from the 0 position, it is just appended:

1. Compare fsPromises.writeFile() behavior:

const fsPromises = require('fs').promises;

const fileName = 'test.txt';

(async function main() {
  try {
    await fsPromises.writeFile(fileName, '123');
    await fsPromises.writeFile(fileName, '0');
    console.log(await fsPromises.readFile(fileName, 'utf8'));
    await fsPromises.unlink(fileName);

    const filehandle = await fsPromises.open(fileName, 'w');
    await fsPromises.writeFile(filehandle, '123');
    await fsPromises.writeFile(filehandle, '0');
    await filehandle.close();
    console.log(await fsPromises.readFile(fileName, 'utf8'));
    await fsPromises.unlink(fileName);
  } catch (err) {
    console.error(err);
  }
})();

0
1230

2. See the same filehandle.writeFile() behavior:

const fsPromises = require('fs').promises;

const fileName = 'test.txt';

(async function main() {
  try {
    const filehandle = await fsPromises.open(fileName, 'w');
    await filehandle.writeFile('123');
    await filehandle.writeFile('0');
    await filehandle.close();
    console.log(await fsPromises.readFile(fileName, 'utf8'));
    await fsPromises.unlink(fileName);
  } catch (err) {
    console.error(err);
  }
})();

1230

[vsemozhetbyt]

cc @jasnell re fsPromises API divergence ^^^.

[vsemozhetbyt]

Maybe the easiest solution would be to call .[f]truncate() on a filehandle before writing in all cases? If so, it would be semver major and we better hurry to fix before v11 RC will be cut.

[vsemozhetbyt]

Adding v11 milestone to be on the safe side. Feel free to remove if we should document these unexpectednesses rather than fix them.

[vsemozhetbyt]

@nodejs/fs if we want to fix this behavior in a semver-major way, we have just a week till v11 semver-major cut-off (October 2nd).

[thefourtheye]

diff --git a/lib/fs.js b/lib/fs.js
index 3302cfe0bf..2fe9ec5bbd 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1193,7 +1193,17 @@ function writeFile(path, data, options, callback) {
       data : Buffer.from('' + data, options.encoding || 'utf8');
     const position = /a/.test(flag) ? null : 0;

-    writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback);
+    if (isUserFd && !/a/.test(flag)) {
+      ftruncate(fd, (err) => {
+        if (err) {
+          return callback(err);
+        }
+        writeAll(
+          fd, isUserFd, buffer, 0, buffer.byteLength, position, callback);
+      });
+    } else {
+      writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback);
+    }
   }
 }

@@ -1203,6 +1213,9 @@ function writeFileSync(path, data, options) {

   const isUserFd = isFd(path); // file descriptor ownership
   const fd = isUserFd ? path : fs.openSync(path, flag, options.mode);
+  if (isUserFd && !/a/.test(flag)) {
+    ftruncateSync(fd);
+  }

   if (!isArrayBufferView(data)) {
     data = Buffer.from('' + data, options.encoding || 'utf8');

This rough attempt seems to fix it and all our current tests pass.

[jasnell]

This will need to land by Saturday if it's going to make the 11.0.0 milestone.

[thefourtheye]

@vsemozhetbyt Can this be closed now, as we concluded #23433 and documentation update is pending in #25080?

[vsemozhetbyt]

Thank you for handling this.