fs: don't conflate data and callback in fs.appendFile

[seishun]

Fixes#11595.

Before:

> fs.writeFile('foo', console.log) undefined > null > fs.appendFile('foo', console.log) undefined > null 

After:

> fs.writeFile('foo', console.log) undefined > (node:13028) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated. > fs.appendFile('foo', console.log) undefined > (node:13028) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated. 

Will add a test if there are no objections to the fix.

Suggestions regarding the commit message are welcome.

Checklist

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

Affected core subsystem(s)

fs

[vsemozhetbyt]

@seishun
Should we ignore that v4.* does not support default parameters, so this fix cannot be backported?

[jasnell]

v4 is heading into maintenance mode in just over a week. We shouldn't need to worry about the inability to backport. The real concern, however, is that param defaults are not very optimized in v6 or v7. They likely won't see significant performance improvement until the new ignition toolchain lands in v8 5.9.

@seishun ... have you benchmarked these changes at all?

[seishun]

Never written benchmarks before, so I just copied readfile.js and changed read into write. Results:

 improvement confidence p.value fs\\writefile.js concurrent=1 len=1024 dur=5 -0.10 % 0.9584959 fs\\writefile.js concurrent=1 len=16777216 dur=5 -0.25 % 0.8447709 fs\\writefile.js concurrent=10 len=1024 dur=5 0.67 % 0.4847459 fs\\writefile.js concurrent=10 len=16777216 dur=5 -0.70 % 0.5841252 

No difference basically.

[seishun]

Tried replacing the default argument with || options, there was no difference either:

Saving 7 x 7 in image improvement confidence p.value fs\\writefile.js concurrent=1 len=1024 dur=5 0.48 % 0.7482698 fs\\writefile.js concurrent=1 len=16777216 dur=5 -1.39 % 0.2153722 fs\\writefile.js concurrent=10 len=1024 dur=5 -0.47 % 0.5425692 fs\\writefile.js concurrent=10 len=16777216 dur=5 1.07 % 0.4011331 

@vsemozhetbyt can you comment? Maybe I'm doing something wrong.

[vsemozhetbyt]

@seishunIIRC, we try to fix it not for performance, but for API correctness, so there can be no difference in benchmarks.

[seishun]

@vsemozhetbyt Yes, but your PR caused a performance degradation, and mine apparently doesn't. Which is what confuses me.

[jasnell]

The key thing with the benchmarks is that we don't want to see a significant loss. It's perfectly fine for things to remain the same :-)

@seishun ... which versions were you comparing inn the benchmark runs above? I'd like to do a comparison run on my end

[vsemozhetbyt]

@seishun That PR performance degradation was murky and I did not understand it. However, we changed things differently, so v8 may like your variant more than mine)

[seishun]

@vsemozhetbyt I tried your variant too, but there was no difference.

Now I'm more concerned about consistency. It seems we don't use default arguments anywhere in node.js, so maybe we shouldn't deviate from that here either. @nodejs/collaborators thoughts?

Also, do we want to add this silly benchmark (last commit here)?

[vsemozhetbyt]

@seishun Did you try with your benchmark or my ones (there are two there: with concurrent writes in the commit and with linear writes in this comment)? Maybe it is something wrong with my benchmarks.

[seishun]

@vsemozhetbyt There's a difference with your benchmark.

With a default argument:

 improvement confidence p.value fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000 -13.02 % *** 1.215164e-09 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000 -12.72 % *** 1.456396e-08 

With || options:

 improvement confidence p.value fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000 -10.62 % *** 1.591341e-06 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000 -13.88 % *** 4.443414e-09 

The difference is that your benchmark doesn't wait for the callback, which I guess makes it less realistic than mine.

@jasnell

@seishun ... which versions were you comparing inn the benchmark runs above? I'd like to do a comparison run on my end

I compared master and this PR rebased on master.

[vsemozhetbyt]

@seishun If I get it right, the benchmark with concurrent writes (in the commit) doesn't use callbacks, while the benchmark with linear writes (in this comment) does use callbacks. However, I had downgrade with both(

[silverwind]

fs change lgtm

[addaleax]

Now I'm more concerned about consistency. It seems we don't use default arguments anywhere in node.js, so maybe we shouldn't deviate from that here either. @nodejs/collaborators thoughts?

If it benchmarks okay (which it seems to by now?) I’m in favour of using default arguments, most of the time it increases readability.

[jasnell]

Default arguments seem to have improved significantly recently but I'd would still likely avoid them in hot code for a bit longer.

[jasnell]

Also keep in mind that there are other side effects... Such as the 'length' property of the function not counting params with defaults.

[vsemozhetbyt]

How can we proceed with this fix?

[mcollina]

Ahum, I'm slightly towards -1 on this. Not because of the change in itself, but because writeFile requires data to be a string, a buffer or a uint8array. I think we should check that, and throw an error if that's not the case.
To me, this PR is ok, but it does not fix the problem in the description, in fact it makes it even less clear.

[seishun]

To me, this PR is ok, but it does not fix the problem in the description, in fact it makes it even less clear.

Please provide a test case that isn't fixed by this PR.

[mcollina]

To me,

>fs.writeFile('foo',console.log)undefined>(node:13028)[DEP0013]DeprecationWarning: Callinganasynchronousfunctionwithoutcallbackisdeprecated.

Should throw because of a missing data object, not give a warning because of a missing callback. The missing callback warning should still be there, in case we call writeFile(path, data). This PR is not improving the ergonomics of API: the user clearly did not put a valid data object in (string, data, Uint8Array), and we should throw on that, not give a warning about a missing callback.

[seishun]

@mcollina It's true that node allows you to specify any value as data:

C:\Users\Nikolai\Downloads\node>node > fs.writeFile('foo', console.log, function(){}); undefined > (To exit, press ^C again or type .exit) > C:\Users\Nikolai\Downloads\node>cat foo function (){[native code] } 

But I think it's a separate issue, and it's not clear cut whether we want to be more strict about it.

[mcollina]

The problem in:

>fs.writeFile('foo',console.log)undefined>(node:13028)[DEP0013]DeprecationWarning: Callinganasynchronousfunctionwithoutcallbackisdeprecated.

is that console.log is a function, which can be used as a callback. So, as a user I would think that I did put the callback in there. So, to fix this I think we should be more strict and require that what we write is either a String, UInt8Array, or Buffer.

[seishun]

So, to fix this I think we should be more strict and require that what we write is either a String, UInt8Array, or Buffer.

This automatic conversion goes way back to fa829b0 and nodejs/node-v0.x-archive#657. I'm not sure if it's something worth changing at this point.

@nodejs/collaborators opinions?

[seishun]

It turns out that @mscdex has already done part of the work in e8a4290. So this PR will just make things consistent between fs.writeFile and fs.appendFile.

[refack]

😸

[refack]

[not very thought out suggestion]
Maybe count bytes?
If I understand this correctly this is supposed to benchmarks node mechanics not the OS, so number of runs would be my intuition too, but then there is weak correlation between the len arg to the results.
If you count bytes, the number of runs can be deduced by calculating result / len, so we don't lose information, and it might be easier to see the ratio of node time / OS time

[seishun]

I'd prefer to just not add this benchmark TBH. I only added it to demonstrate that default arguments don't cause performance regressions, but I'm not using them anymore. I should have just gisted it instead I guess.

[refack]

Also a valid option IMHO

[refack]

Since this asserts https://nodejs.org/api/fs.html#fs_fs_writefile_file_data_options_callback I think an assert.throws test case should be added to test/parallel/test-fs-write-file.js and test/parallel/test-fs-append-file.js

[seishun]

@refack what test case exactly? It doesn't introduce any new exceptions.

[refack]

@refack what test case exactly? It doesn't introduce any new exceptions.

Forgot we reverted throwing in #12976, but it should emit warning
(node:15000) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated.

[seishun]

Added a test, PTAL.

[refack]

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

[seishun]

The failures seem unrelated?

[refack]

OSX - known #13559
AIX - known #13577

freeBSD - parallel/test-process-external-stdio-close
Maybe so rerunning...
https://ci.nodejs.org/job/node-test-commit-freebsd/9644/

[refack]

freeBSD was a flake - #13586

[MylesBorins]

Should this be backported to v6.x?

[seishun]

If #12456 was backported, then this should be too.