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

seishun · 2017-02-28T15:20:46Z

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 · 2017-02-28T18:27:20Z

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

jasnell · 2017-03-17T18:06:55Z

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 · 2017-03-17T19:55:20Z

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 · 2017-03-17T20:30:53Z

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 · 2017-03-17T20:55:44Z

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

seishun · 2017-03-17T20:57:55Z

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

jasnell · 2017-03-17T20:59:14Z

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 · 2017-03-18T00:59:34Z

@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 · 2017-03-18T09:11:06Z

@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 · 2017-03-18T10:55:40Z

@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 · 2017-03-18T13:53:08Z

@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 · 2017-03-18T14:32:05Z

@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 · 2017-03-19T00:42:54Z

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 · 2017-03-19T00:46:19Z

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

jasnell · 2017-03-19T00:47:29Z

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

vsemozhetbyt · 2017-04-17T22:09:19Z

How can we proceed with this fix?

mcollina · 2017-04-17T22:32:44Z

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 · 2017-04-18T10:07:11Z

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 · 2017-04-18T11:50:09Z

To me,

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

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 · 2017-06-02T14:30:41Z

@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 · 2017-06-02T14:45:14Z

The problem in:

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

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 · 2017-06-02T15:05:04Z

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 · 2017-06-08T16:48:38Z

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 · 2017-06-08T17:32:51Z

benchmark/fs/writefile.js

@@ -0,0 +1,44 @@
+// Call fs.writeFile over and over again really fast.
+// Then see how many times it got called.
+// Yes, this is a silly benchmark.  Most benchmarks are silly.


[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

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.

Also a valid option IMHO

refack · 2017-06-08T17:44:55Z

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 · 2017-06-08T17:50:50Z

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

refack · 2017-06-08T18:36:28Z

@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 · 2017-06-09T13:21:02Z

Added a test, PTAL.

refack · 2017-06-09T18:03:53Z

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

seishun · 2017-06-09T19:33:01Z

The failures seem unrelated?

refack · 2017-06-09T19:59:05Z

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 · 2017-06-09T20:14:59Z

freeBSD was a flake - #13586

PR-URL: #11607 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-07-17T18:26:56Z

Should this be backported to v6.x?

seishun · 2017-07-17T18:54:16Z

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

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 28, 2017

silverwind approved these changes Mar 18, 2017

View reviewed changes

addaleax approved these changes Mar 19, 2017

View reviewed changes

vsemozhetbyt mentioned this pull request Mar 23, 2017

Some possible bugs concerning fs.appendFile() and fs.writeFile() #11595

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

seishun changed the title ~~fs: don't conflate data and callback in arguments~~ fs: don't conflate data and callback in fs.appendFile Jun 8, 2017

refack approved these changes Jun 8, 2017

View reviewed changes

cjihrig approved these changes Jun 9, 2017

View reviewed changes

refack approved these changes Jun 9, 2017

View reviewed changes

seishun closed this Jun 10, 2017

seishun deleted the fs-params branch June 10, 2017 18:27

seishun restored the fs-params branch June 10, 2017 18:28

seishun deleted the fs-params branch June 10, 2017 18:30

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax mentioned this pull request Jun 21, 2017

v8.1.3 proposal #13861

Merged

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 14, 2017

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

aliscco mentioned this pull request Nov 29, 2023

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#786

Open

aliscco mentioned this pull request May 15, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1513

Open

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

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

Conversation

seishun commented Feb 28, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Feb 28, 2017 • edited Loading

jasnell commented Mar 17, 2017

seishun commented Mar 17, 2017

seishun commented Mar 17, 2017

vsemozhetbyt commented Mar 17, 2017

seishun commented Mar 17, 2017

jasnell commented Mar 17, 2017 • edited Loading

vsemozhetbyt commented Mar 18, 2017

seishun commented Mar 18, 2017

vsemozhetbyt commented Mar 18, 2017 • edited Loading

seishun commented Mar 18, 2017 • edited Loading

vsemozhetbyt commented Mar 18, 2017 • edited Loading

silverwind left a comment

Choose a reason for hiding this comment

addaleax commented Mar 19, 2017

jasnell commented Mar 19, 2017

jasnell commented Mar 19, 2017

vsemozhetbyt commented Apr 17, 2017

mcollina commented Apr 17, 2017

seishun commented Apr 18, 2017

mcollina commented Apr 18, 2017

seishun commented Jun 2, 2017 • edited Loading

mcollina commented Jun 2, 2017

seishun commented Jun 2, 2017

seishun commented Jun 8, 2017

refack Jun 8, 2017

Choose a reason for hiding this comment

refack Jun 8, 2017

Choose a reason for hiding this comment

seishun Jun 8, 2017

Choose a reason for hiding this comment

refack Jun 8, 2017

Choose a reason for hiding this comment

refack commented Jun 8, 2017

seishun commented Jun 8, 2017

refack commented Jun 8, 2017

seishun commented Jun 9, 2017

refack commented Jun 9, 2017

seishun commented Jun 9, 2017

refack commented Jun 9, 2017 • edited Loading

refack commented Jun 9, 2017

MylesBorins commented Jul 17, 2017

seishun commented Jul 17, 2017

seishun commented Feb 28, 2017 •

edited

Loading

vsemozhetbyt commented Feb 28, 2017 •

edited

Loading

jasnell commented Mar 17, 2017 •

edited

Loading

vsemozhetbyt commented Mar 18, 2017 •

edited

Loading

seishun commented Mar 18, 2017 •

edited

Loading

vsemozhetbyt commented Mar 18, 2017 •

edited

Loading

seishun commented Jun 2, 2017 •

edited

Loading

refack commented Jun 9, 2017 •

edited

Loading