fs: track file offset in writeFile of fs/promises

[thefourtheye]

If the file offset is not tracked with position in the write calls
all the data will be always written at the end of the file.

This is problematic when we pass file descriptors to writeFile because
the the file offset will not be updated. So, if some string is written
to the FD with writeFile and if some other string is written to the
same FD with writeFile without closing and reopening the file, the
contents will be appended instead of writing from the beginning.

---

Simple Reproduction of the Bug

'use strict';
const assert = require('assert');
const fs = require('fs').promises;

(async function() {
    const h = await fs.open('/tmp/test', 'w');
    await h.writeFile('AAAA');
    await h.truncate();
    await h.writeFile('BBB');
    await h.close();
    assert.deepStrictEqual(await fs.readFile('/tmp/test', 'utf8'), 'BBB');
})();

(node:51030) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ '\u0000\u0000\u0000\u0000BBB'
- 'BBB'

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@thefourtheye build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1046/pipeline

[thefourtheye]

cc @nodejs/fs

---

@vsemozhetbyt If this change gets in, I'll propose a fix for #22554

[thefourtheye]

CI: https://ci.nodejs.org/job/node-test-pull-request/17479/

[addaleax]

This might just be me and my unix background, but the current behaviour seems right to me (both here and in #23181)…

[thefourtheye]

@addaleax Either way, one of the examples is not correct, right? Quoting the example from #22554

'use strict';

const fs = require('fs');

const fileName = 'test.txt';
fs.writeFileSync(fileName, '123');
fs.writeFileSync(fileName, '0');
console.log(fs.readFileSync(fileName, 'utf8'));
// 0
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'));
// 023
fs.unlinkSync(fileName);

Another possibility is that we can update the documentation about the file descriptor case.

Personally, I have always expected writeFile* functions to overwrite the file.

[addaleax]

Personally, I have always expected writeFile* functions to overwrite the file.

@thefourtheye A file descriptor is more than a reference to a file, it’s a reference to a file and a position in that file (if the file supports positioning)… but yeah, let’s see what other people think.

[thefourtheye]

@nodejs/fs PTAL.

[thefourtheye]

Bump @nodejs/fs

[sam-github]

Positional writes don't work on streams, try running this variant of writeFile to stdout.

[thefourtheye]

Agreed, if we cannot seek the position in fd, it will fail. Wouldn't that be the expected behaviour in this case?

[sam-github]

This issue was beaten to death in the past. Seeking to position 0 on a fd provided to writeFile may seem like the obviously right thing to do... but doesn't work on pipes.

Pleas read #10853 (comment) for some history, and the ensuing conversation, its about readFile but the issues are the same.

Summary: I believe current behaviour is intended, and if you could get agreement on the behaviour around unseekable fds, changing it would be semver-major.

[thefourtheye]

Today, we define fs.writeFile as

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

So, if the fd represents a file then it has to contain only the data given as one of the parameters to fs.writeFile.

If we cannot seek to a particular position in the file with a file descriptor, then an error would be the expected behaviour I believe. Maybe we can explicitly mention in the docs.

Other option would be to explicitly introduce a check in these functions to make sure that the fd actually corresponds to a file.

I understand that all these are breaking changes.

[thefourtheye]

@sam-github This PR is not semver-major as the fs promises APIs are still experimental, I believe. The actual PR to fix the behaviour of writeFile is in #23181 and that is tagged as semver-major.

[sam-github]

@thefourtheye Did you read the history in #10853? Some people expected seeking, some people did not. One node test at that time, perhaps accidently, required readFile to work on a stdin. It would be sensible if the equivalent test existed for writeFile, but it possibly does not.

Quoting the docs is not enormously compelling, they were not sufficiently updated when the fd capability was shoe-horned into the existing API.

I don't have a strong opinion on seek or not anymore, both alternatives have valid use-cases, but I do have one strong opinion: if writeFile seeks, so should readFile. Please do not make them diverge. Note: I don't know if this PR does or not, I haven't gone to check the current state of readFile.

[thefourtheye]

@sam-github Thanks, I read the history from #10853 and the linked issues. We'll make sure that the behaviour is consistent with both readFile and writeFile.

I don't know if this PR does or not, I haven't gone to check the current state of readFile.

Neither of these PRs change the readFile. Perhaps, lets come to a conclusion and implement it, if needed.

[jasnell]

This will need to land by Saturday in order to make it into 11.0.0.
Note that this is blocking a semver-major from landing that is also on the 11.0.0 milestone.

[Trott]

@thefourtheye Is the hope to still land this? Or has this been superseded by other things?

[thefourtheye]

As per the decision in #23433, the current behaviour is expected. So closing this.