fs.writeFile doesn't work with non-seekable files

[mildsunrise]

• Version: 13.7.0
• Platform: Linux
• Subsystem: fs

I see that fs.writeFile* fails with non-seekable files, because it does a positioned write:

fs.writeFileSync('/sys/kernel/debug/tracing/trace_clock', 'mono')

openat(AT_FDCWD, "/sys/kernel/debug/tracing/trace_clock", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 19
pwrite64(19, "mono", 4, 0)              = -1 ESPIPE (Illegal seek)

fs.readFile works correctly since it does a regular read, and I also found that createWriteStream was fixed to work with non-seekable files (#19329).
Is this known/intentional? If not, would a fix be accepted? (I haven't investigated further yet)

[addaleax]

👍

I think fixing this might be as straightforward as switching the position parameter from 0 to null in the writeFile() function.

The only thing that I could see being tricky about this is that this changes behaviour when an fd is being passed where the write position had already been advanced before. Maybe it would be best to only perform this change for freshly opened files, i.e. when a path is being passed?

[mildsunrise]

Oh true, I didn't remember that fds are accepted... yes, for fds it would change behaviour, but at the same time it's a bug people would likely not rely on, and it's inconsistent with fs.write.

if we don't want to change behaviour, I suppose we can fix it only for paths like you said, and then a semver-major full fix?

[mildsunrise]

btw, it looks like the behaviours are different between writeFile:

node/lib/fs.js

Line 1300 in 9c70292

const position = /a/.test(flag) ? null : 0;

and writeFileSync, which does avoid setting pos = 0 for fds:

node/lib/fs.js

Line 1321 in 9c70292

let position = (/a/.test(flag) || isUserFd) ? null : 0;

[addaleax]

@mildsunrise Yeah, that’s … not great. I’m not sure what to do about that, besides what you suggested above – first only fix this for paths, then make behaviour consistent in a semver-major change.

[mildsunrise]

I overlooked the code again... it turns out the semver-major change we were talking about was already decided in #23433 and landed in #23709 at v12. So we just need to fix it for paths :) I'll make a PR