Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: add promises API #18297

Closed
wants to merge 4 commits into from
Closed

fs: add promises API #18297

wants to merge 4 commits into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Jan 22, 2018

fs.Promises, the actual PR (replaces #17739)

/cc @mcollina @addaleax

/cc @bmeurer and @nodejs/v8 @nodejs/chakracore ... it would be excellent if we could have y'all take a look at this and do some performance testing.

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
Affected core subsystem(s)

fs

@jasnell jasnell changed the title Fs promises fs: add promises API Jan 22, 2018
@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 22, 2018

@addaleax ... because I know you'll ask.. this intentionally does not do the util.promisify() customization yet for a couple reasons. 1) because there are folks who use that in production today, I'd like to make sure this implementation is solid before switching... and 2) there's a discrepancy between methods like fs.fstat() and fs.promises.fstat() in that the former takes the numeric file descriptor and the latter takes the new FileHandle object.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 22, 2018

My intent would be to land this as an experimental feature

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 22, 2018

Just fyi.. benchmark comparison for bench-stat vs bench-stat-promises (running in a vm on a laptop...)

$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 20,714.827782562465
fs/bench-stat-promise.js statType="lstat" n=200000: 15,589.439105836866
fs/bench-stat-promise.js statType="stat" n=200000: 22,186.395158011303

$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 13,882.790153144382
fs/bench-stat-promise.js statType="lstat" n=200000: 15,744.878475049993
fs/bench-stat-promise.js statType="stat" n=200000: 17,180.365568781977

$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 20,064.173934086277
fs/bench-stat-promise.js statType="lstat" n=200000: 16,715.911703541507
fs/bench-stat-promise.js statType="stat" n=200000: 17,014.91350175299

$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 19,148.535379755922
fs/bench-stat.js statType="lstat" n=200000: 17,326.004694161828
fs/bench-stat.js statType="stat" n=200000: 18,626.09603864018

$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 14,399.220751034958
fs/bench-stat.js statType="lstat" n=200000: 14,749.28092887841
fs/bench-stat.js statType="stat" n=200000: 21,620.088712923043

$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 19,873.273062278913
fs/bench-stat.js statType="lstat" n=200000: 23,040.86746115833
fs/bench-stat.js statType="stat" n=200000: 27,742.886612762228

doc/api/fs.md Outdated
`fs.promises.open()` method.

Unlike the callback-based `fs.fstat()`, `fs.fchown()`, `fs.fchmod()`,
`fs.ftruncate()`, `fs.read()`, and `fs.write()`, operations -- all of which
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

Is it supposed to be an exhaustive list? If so, maybe fs.appendFile(), fs.fdatasync(), fs.fsync(), fs.readFile(), fs.futimes(), and fs.writeFile() should be added.

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

Not exhaustive, will add some clarifying language

doc/api/fs.md Outdated
-->

* Returns: {Promise} A `Promise` that will be resolved once the underlying
file descriptor is closed, or will reject if an error occurs while closing.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

will reject -> will be rejected?

doc/api/fs.md Outdated
* `fs.constants.X_OK` - `path` can be executed by the calling process. This has
no effect on Windows (will behave like `fs.constants.F_OK`).

If the accessible check is successful, the `Promise` is resolved with no
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

accessible check -> access check / accessibility check ?

doc/api/fs.md Outdated
@@ -3446,6 +4203,7 @@ The following constants are meant for use with the [`fs.Stats`][] object's
[`AHAFS`]: https://www.ibm.com/developerworks/aix/library/au-aix_event_infrastructure/
[`Buffer.byteLength`]: buffer.html#buffer_class_method_buffer_bytelength_string_encoding
[`Buffer`]: buffer.html#buffer_buffer
[`FileHandle`]: #fs_class_filehandle
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

It is [FileHandle] (without backticks) in the signatures, so this needs to be synced.

Or this type can be added here to spare some doc bytes)

doc/api/fs.md Outdated

The `file` may be specified as a `FileHandle` that has been opened
for appending (using `fs.promises.open()`). The `FileHandle` will
not be closed automatically.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

Does this mean that any FileHandle opened for appending will not be closed automatically? If so, should this be added to FileHandle description as an exception?

doc/api/fs.md Outdated
`bytesWritten` specifies how many _bytes_ were written from `buffer`.

Note that it is unsafe to use `fs.write` multiple times on the same file
without waiting for the `Promise` to resolve. For this scenario,
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

"to resolve" -> "to be resolved (or rejected)"?

doc/api/fs.md Outdated
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
* `flag` {string} **Default:** `'w'`

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

Missing "Returns: ..."?

doc/api/fs.md Outdated
The `encoding` option is ignored if `data` is a buffer. It defaults
to `'utf8'`.

If `options` is a string, then it specifies the encoding. Example:
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

Obsolete " Example:"

doc/api/fs.md Outdated

Any specified `FileHandle` has to support writing.

It is unsafe to use `fs.writeFile` multiple times on the same file without
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

fs.writeFile -> fs.promises.writeFile()

doc/api/fs.md Outdated
waiting for the `Promise` to be resolved (or rejected).

If a `FileHandle` is specified as the `file`, it will not be closed
automatically.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jan 23, 2018

"it will not be closed automatically" — if this is true, is it worth to be mentioned as an exception in the FileHandle description?

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

good point, will need to figure out the different wording here. The FileHandle will be closed if it is allowed to gc .... but if a FileHandle is provided to this function, the function itself will not call filehandle.close(), it's still up to the user to do that.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jan 23, 2018

  1. This is going to be a talkative PR, so please, feel free to delete any of my comments if they are not appropriate or will not be hidden after addressing (or even if they will be hidden).

  2. Is the omission of the fs.write(fd, string[, position[, encoding]], callback) variant intended?

  3. If I get it right, the promises API doc part is not as full as the callback API part (some examples and notes are omitted). Is it worth to mention this in the intro part? Like "for more info see callback API counterparts"?

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 23, 2018

@vsemozhetbyt ... as always I love your reviews of my doc commits. Thank you for being so thorough. Yes, the omission of the fs.write(fd, string, position, encoding, callback) variant is intentional.

I plan to go back and add better examples either in an iteration of this PR or a future one.

lib/fs.js Outdated
}

fs.promises = {
async access(path, mode = fs.F_OK) {
Copy link
Contributor

@ofrobots ofrobots Jan 23, 2018

Given that you never await anything, does this need to be async?

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

To catch errors thrown, yes.

Copy link
Contributor

@ofrobots ofrobots Jan 23, 2018

@jasnell, can you elaborate? For my education, how does making the function async help with catching thrown errors? My understanding is that async merely enables the use of the await keyword.

Copy link
Member

@targos targos Jan 23, 2018

Any error thrown within an async function is automatically caught and causes rejection of the promise returned by the function. With a regular function you could have two cases: synchronous throw or rejection depending on where the error happens.

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

Yeah, what @targos said... for instance, try the following cases:

function foo() {
  throw new Error('foo');  // error is thrown synchronously
  return new Promise(() => {});
}

async function bar() {
  throw new Error('bar'); // error causes promise rejection
  return new Promise(() => {});
}

@jinwoo
Copy link
Contributor

@jinwoo jinwoo commented Jan 23, 2018

Thanks for this. This is great.

Can we have a shorter namespace though? Something like fs.p (or fs.prom if fs.p is too terse)? It'd be painful if I have to type fs.promises all the time.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jan 23, 2018

@jasnell You may also need to update the appropriate counterparts according to that PR (I've found those missing types reviewing this PR :) ).

@bmeurer
Copy link
Member

@bmeurer bmeurer commented Jan 23, 2018

@jasnell Your benchmark run seems to suggest that the promise performance is already quite good compared to the callback version. Although that is of course a micro-benchmark. Do you have a representative (macro-)benchmark?

@bmeurer
Copy link
Member

@bmeurer bmeurer commented Jan 23, 2018

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 23, 2018

Yep and rightfully so given that they mostly use exactly the same internal mechanisms. The differences are quite minimal thankfully. Where they are different is in the creation of the additional promise and the additional overhead of the promise hook, both of which are fairly minimal.

I do not yet have a macro benchmark in place. It'll likely take bit to get that.

One thing I did notice is that there are about twice as many GC runs in the promises microbenchmark, which is not at all surprising but is certainly worth noting.

@bmeurer
Copy link
Member

@bmeurer bmeurer commented Jan 23, 2018

@jasnell Did you see #17739 (comment) from @gsathya? That might be worth investigating.

src/env.cc Outdated
cb.cb_(this, cb.data_);
if (try_catch.HasCaught()) {
Copy link
Member

@joyeecheung joyeecheung Jan 23, 2018

It is worth updating the comment of SetImmediate that throwing errors in a native immediate callback would crash the process.

int ret = uv_fs_close(env()->event_loop(), &req, fd_, nullptr);
uv_fs_req_cleanup(&req);

struct err_detail { int ret; int fd; };
Copy link
Member

@joyeecheung joyeecheung Jan 23, 2018

Why not move this outside?

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

Because it's only used here. If it turns out to be useful elsewhere later, moving it is easy :-)

src/node_file.cc Outdated
AsyncCall(env, args, "open", UTF8, AfterOpenFileHandle,
uv_fs_open, *path, flags, mode);
} else {
SYNC_CALL(open, *path, *path, flags, mode)
Copy link
Member

@joyeecheung joyeecheung Jan 23, 2018

Can you use SyncCall here?

src/node_file.h Outdated

private:
bool finished_ = false;
double statFields_[14] {};
Copy link
Member

@joyeecheung joyeecheung Jan 23, 2018

Why making it an array here though? Maybe use a unique_ptr and only allocates memory and reset it when FillStatsArray is called?

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

to avoid an extra allocation. should be pretty cheap doing it this way overall.

}

const base = path.resolve(common.tmpDir, 'FOO');
assert(
Copy link
Member

@joyeecheung joyeecheung Jan 23, 2018

Does this work on windows? common.tmpDir would contain \ so it should not be able to be passed into new RegExp like that.

Copy link
Member Author

@jasnell jasnell Jan 23, 2018

heh, good point. will have to double check

@targos
Copy link
Member

@targos targos commented Jan 23, 2018

I don't know if we should discuss this here but I have a concern with the interaction of fs.promises and ESM (related to #18131).
We are committed to making ESM a first class feature in Node, but with the current way things are going, it will be difficult to provide the Promise APIs in an ESM-friendly way (no way to directly import a specific promisified method).
It seems the only way would be to have the Promise API in another module. Examples: require('fs.promise'), require('fs/promise')

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 23, 2018

@bmeurer ... Yeah, I'm working that way but wanted to get all of the js functions impl'd to make sure we would be able to completely eliminate creating the FSReqPromise object in js. I've got a way forward on that now that should work nicely, tho I'll have to test that it does not negatively impact the perf of the callback approach first.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 23, 2018

@targos ... definitely a valid concern, will have to think about that a bit more. FWIW, we would have precisely the same issue with path.win32and path.posix, and likely several other bits of core API, so I'm not sure if solving it just for fs.promises is the right thing or if we should come with a more general solution.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Jan 23, 2018

@bmeurer ... updated to eliminate the creation of FSReqPromise in JS land, Definitely makes a difference.

james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 21,609.73968730913
fs/bench-stat-promise.js statType="lstat" n=200000: 13,606.886508071122
fs/bench-stat-promise.js statType="stat" n=200000: 20,361.793179169545
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 19,382.167235983314
fs/bench-stat-promise.js statType="lstat" n=200000: 14,694.606891503132
fs/bench-stat-promise.js statType="stat" n=200000: 13,698.616828686227
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 28,453.65135200578
fs/bench-stat-promise.js statType="lstat" n=200000: 13,697.273150949744
fs/bench-stat-promise.js statType="stat" n=200000: 18,633.918977446763
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 16,870.88056950238
fs/bench-stat-promise.js statType="lstat" n=200000: 13,472.76114383914
fs/bench-stat-promise.js statType="stat" n=200000: 26,460.504276421147
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 36,322.48835345608
fs/bench-stat.js statType="lstat" n=200000: 15,002.883243224593
fs/bench-stat.js statType="stat" n=200000: 22,319.499506552078
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 32,136.6980781016
fs/bench-stat.js statType="lstat" n=200000: 14,883.592505458588
fs/bench-stat.js statType="stat" n=200000: 25,631.456606611602
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat.js
fs/bench-stat.js statType="fstat" n=200000: 31,111.359958434878
fs/bench-stat.js statType="lstat" n=200000: 22,671.7262379716
fs/bench-stat.js statType="stat" n=200000: 17,104.025292210692
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 30,095.77601510028
fs/bench-stat-promise.js statType="lstat" n=200000: 24,965.011103171837
fs/bench-stat-promise.js statType="stat" n=200000: 27,054.78894406488
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 30,281.66760510009
fs/bench-stat-promise.js statType="lstat" n=200000: 23,380.61184604834
fs/bench-stat-promise.js statType="stat" n=200000: 27,438.146664427066
james@ubuntu:~/node/node$ ./node benchmark/fs/bench-stat-promise.js
fs/bench-stat-promise.js statType="fstat" n=200000: 26,104.278785435945
fs/bench-stat-promise.js statType="lstat" n=200000: 20,969.553527682114
fs/bench-stat-promise.js statType="stat" n=200000: 13,553.763324970801

@bmeurer
Copy link
Member

@bmeurer bmeurer commented Jan 23, 2018

@jasnell Cool! 👍 Kudos to @gsathya for the finding!

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 23, 2018

@targos makes a great point in #18297 (comment).

I'd be a definite +1 on require('fs/promise') / require('fs-promise') / require('fs.promise') require('fsp') or similar to ensure ES module compatibility. ES module parity should be a primary consideration for all API changes at this point.

@guybedford
Copy link
Contributor

@guybedford guybedford commented Jan 23, 2018

@jasnell if we were implementing path.posix today the same suggestion would apply, but given it's a stable API it probably doesn't make sense to change now - import { posix } from 'path' seems less costly as it's a much rarer API use case anyway than import { readFile } from 'fs-promise'.

jasnell added a commit that referenced this issue Feb 6, 2018
Initial set of fs.promises APIs with documentation and one
benchmark.

PR-URL: #18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Member Author

@jasnell jasnell commented Feb 6, 2018

Landed in df34029, 7154bc0, 85b37db and 329fc78. woot!

@jasnell jasnell closed this Feb 6, 2018
vsemozhetbyt added a commit that referenced this issue Feb 6, 2018
This seems to be slipped in #18297

PR-URL: #18599
Refs: #18309
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
addaleax added a commit to addaleax/node that referenced this issue Mar 6, 2018
PR-URL: nodejs#18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue Mar 6, 2018
Backport-PR-URL: #19185
PR-URL: #18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue Mar 6, 2018
Backport-PR-URL: #19185
PR-URL: #18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue Mar 6, 2018
Backport-PR-URL: #19185
PR-URL: #18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue Mar 6, 2018
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* lib:
  - v8_prof_processor works again 🎉 (Anna Henningsen)
    #19059
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - handle exceptions in env-\>SetImmediates (James M Snell)
    #18297
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480

PR-URL: #19181
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
MylesBorins added a commit that referenced this issue Mar 7, 2018
Backport-PR-URL: #19185
PR-URL: #18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins added a commit that referenced this issue Mar 7, 2018
Notable Changes:

* crypto:
  - add cert.fingerprint256 as SHA256 fingerprint (Hannes Magnusson)
    #17690
* lib:
  - v8_prof_processor works again 🎉 (Anna Henningsen)
    #19059
* loader:
  - --inspect-brk now works properly for esmodules (Gus Caplan)
    #18949
* src:
  - handle exceptions in env-\>SetImmediates (James M Snell)
    #18297
  - make process.dlopen() load well-known symbol (Ben Noordhuis)
    #18934
* trace_events:
  - add file pattern cli option (Andreas Madsen)
    #18480

PR-URL: #19181
@targos targos mentioned this pull request Mar 24, 2018
3 tasks
alexeykuzmin added a commit to electron/node that referenced this issue Apr 10, 2018
alexeykuzmin added a commit to electron/node that referenced this issue Apr 27, 2018
azu added a commit to azu/node-browser-polyfill-gap that referenced this issue May 1, 2018
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
The `node::fs::FileHandle` object wraps a file descriptor
and will close it on garbage collection along with a
process warning. The intent is to prevent (as much as
possible) file descriptors from being leaked if the user
does not close them explicitly.

PR-URL: nodejs#18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
Initial set of fs.promises APIs with documentation and one
benchmark.

PR-URL: nodejs#18297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MayaLekova added a commit to MayaLekova/node that referenced this issue May 8, 2018
This seems to be slipped in nodejs#18297

PR-URL: nodejs#18599
Refs: nodejs#18309
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet