fs: initial experimental promisified API #15485

jasnell · 2017-09-20T07:47:27Z

Introduces an experimental promises API for the fs module. The API is accessible via fs.async, e.g.

This is intended to land as Experimental.

const { async:fs } = require('fs');

fs.access('.')
  .then(console.log)
  .catch(console.error);

Most fs functions are supported. There are some variations from
the base fs module (for instance, fs.async.realpath() uses the
faster libuv realpath implementation rather than the slower js
implementation.

This does not yet include documentation updates.

Semver-major because this makes substantive changes to error handling in the existing fs module.

Benchmarks are included. For instance,

access

james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 280,078.9048933864
fs/promises-access.js method="promisify" n=200000: 69,310.04884236863
fs/promises-access.js method="promise" n=200000: 240,928.00098102028
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 236,062.80841465757
fs/promises-access.js method="promisify" n=200000: 82,258.05538216804
fs/promises-access.js method="promise" n=200000: 192,776.68164314007
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 112,929.07886994253
fs/promises-access.js method="promisify" n=200000: 136,272.83764802708
fs/promises-access.js method="promise" n=200000: 169,612.60194223037
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 163,294.66290369138
fs/promises-access.js method="promisify" n=200000: 119,130.69228308211
fs/promises-access.js method="promise" n=200000: 185,855.53426089959

copyFile

james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 282,755.1240812897
fs/promises-copyfile.js method="promisify" n=20000: 166,754.91197646034
fs/promises-copyfile.js method="promise" n=20000: 101,068.59267154362
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 264,452.6619289267
fs/promises-copyfile.js method="promisify" n=20000: 84,065.33716046945
fs/promises-copyfile.js method="promise" n=20000: 100,486.89215315646
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 270,616.2934281645
fs/promises-copyfile.js method="promisify" n=20000: 156,420.93141688287
fs/promises-copyfile.js method="promise" n=20000: 103,760.93116221747
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 251,781.945806882
fs/promises-copyfile.js method="promisify" n=20000: 118,330.0739314469
fs/promises-copyfile.js method="promise" n=20000: 266,315.20161791815
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 106,352.46687950206
fs/promises-copyfile.js method="promisify" n=20000: 120,297.63535689937
fs/promises-copyfile.js method="promise" n=20000: 260,925.24326093044
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 283,371.8168790678
fs/promises-copyfile.js method="promisify" n=20000: 154,272.6477009993
fs/promises-copyfile.js method="promise" n=20000: 177,795.42772743877
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 116,029.27694807078
fs/promises-copyfile.js method="promisify" n=20000: 154,155.65571356818
fs/promises-copyfile.js method="promise" n=20000: 92,732.89058694763
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 286,576.9026610743
fs/promises-copyfile.js method="promisify" n=20000: 69,093.89846983271
fs/promises-copyfile.js method="promise" n=20000: 83,583.96891906712
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-copyfile.js
fs/promises-copyfile.js method="legacy" n=20000: 123,446.49834316899
fs/promises-copyfile.js method="promisify" n=20000: 157,543.37448739127
fs/promises-copyfile.js method="promise" n=20000: 150,655.3488844555

As illustrated by the benchmarks, the promise versions can be faster than the traditional callbacks, but the results are inconsistent. This appears largely to do with gc (there are a lot of promise objects created that need to be cleaned up).

Having the Promise version does not obsolete the callback version, as the creation and management of the promise objects can be fairly expensive.

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)

Introduces an experimental promises API for the fs module. The API is accessible via `fs.async`, e.g. ```js const { async:fs } = require('fs'); fs.access('.') .then(console.log) .catch(console.error); ``` Most fs functions are supported. There are some variations from the base fs module (for instance, `fs.async.realpath()` uses the faster libuv realpath implementation rather than the slower js implementation. This does not yet include documentation updates.

jasnell · 2017-09-20T07:53:40Z

Note that these are not util.promisify wrappers. These are implementations of the fs functions with Promises without any wrapping. In some cases, the methods were entirely reimplemented (e.g. readFile), and changes were made at the native layer to resolve promises there where possible. This implementation should not be considered complete, but it is functional enough to land as experimental.

mscdex · 2017-09-20T08:54:53Z

The API is accessible via fs.async

This seems confusing given that non-*Sync fs methods are also asynchronous.

There are some variations from
the base fs module (for instance, fs.async.realpath() uses the
faster libuv realpath implementation rather than the slower js
implementation.

Why?

addaleax

Nice!

Note that these are not util.promisify wrappers. These are implementations of the fs functions with Promises without any wrapping.

In that case, the [util.promifiy.custom] properties on the original functions should still be set to the “proper” async implementation wherever that makes sense

addaleax · 2017-09-20T10:42:25Z

lib/internal/async/fs.js

+            req);
+  } catch (err) {
+    promiseReject(promise, err);
+  }


Have you tried making all of these functions async functions to have implicit try/catch+reject? I think unless you’re having any await in it that’s all there they are.

Yep, was considering that already :-)

addaleax · 2017-09-20T10:42:41Z

lib/fs.js

+  configurable: true,
+  get() {
+    return require('internal/async/fs');
+  }


Why not value + writable: false?

to avoid the experimental warning from printing until someone explicitly goes to use the new API. We can switch this later once it's non-experimental

addaleax · 2017-09-20T10:46:29Z

src/node_file.cc

+    if (promise->IsPromise()) {
+      if (promise.As<Promise>()->State() != Promise::kPending) return;
+      Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>();
+      resolver->Resolve(context, arg).FromJust();


Due to #5691 you’ll want an InternalCallbackScope around this … ideally move the class definition to node_internals.h in the same way it’s done in #15428 to avoid merge conflicts :)

ah yes, thanks for the reminder :-)

fwiw, how and where to use InternalCallbackScope is about as clear as mud right now :-( ... any chance of having some docs written up on it? Even if only in the form of some code comments.

Yea, sure. From node.h:

* `MakeCallback()` is a wrapper around this class as well as * `Function::Call()`. Either one of these mechanisms needs to be used for * top-level calls into JavaScript (i.e. without any existing JS stack).

do you have any specific suggestions for how to improve that? should resolving promise state be mentioned explicitly?

I'll work up some language.

addaleax · 2017-09-20T10:46:55Z

src/node_file.cc

+    Local<Value> promise =
+        object()->Get(context, env->oncomplete_string()).ToLocalChecked();
+    if (promise->IsPromise()) {
+      if (promise.As<Promise>()->State() != Promise::kPending) return;


When is this not true? Can you make this a check or, alternatively, add a comment?

+1 .. yeah, was already looking at making this a CHECK. It should always be true.

addaleax · 2017-09-20T10:47:21Z

src/node_file.cc

+      Local<Value> argv[2];
+      argv[0] = Null(env->isolate());
+      argv[1] = arg;
+      MakeCallback(env->oncomplete_string(), 2, argv);


arraysize(argv) :)

jasnell · 2017-09-20T14:19:41Z

@mscdex

This seems confusing given that non-*Sync fs methods are also asynchronous.

Yeah, I'm thinking I'll just change it to fs.promises.*

There are some variations from the base fs module (for instance, fs.async.realpath() uses the faster libuv realpath implementation rather than the slower js implementation.

Why?

Performance mainly. And lack of backwards compatibility concerns. The key reason we ended up having to revert back to the old realpath implementation is because changing broke existing code. We don't have that issue here. Also, if we kept the js implementation, we wouldn't really gain any performance advantages over simply doing util.promisify(fs.realpath) given the way it is implemented.

addaleax · 2017-09-20T14:21:19Z

Fwiw, I think it might be cleaner to introduce fs.fastRealpath() for that, maybe?

geek · 2017-09-20T14:32:21Z

lib/fs.js

 const getPathFromURL = internalURL.getPathFromURL;

+const {


What is the issue with accessing internalFs. For example realpathCacheKey is only used in one location, why create a module level variable to manage instead of being explicit and accessing it from internalFs the one time?

BridgeAR · 2017-09-20T14:09:29Z

lib/fs.js

@@ -283,35 +190,41 @@ fs.access = function(path, mode, callback) {
    throw new errors.TypeError('ERR_INVALID_CALLBACK');
  }

-  if (handleError((path = getPathFromURL(path)), callback))
-    return;
+  handleError(path = getPathFromURL(path));


Hm, my first thought when I saw the change to handle errors sync was that it is a good idea but after having a second thought I am not so sure about that anymore. The path could definitely be passed in by users and therefore handling the error in the callback would probably be the best thing to stick to?

Currently, argument validation error handling is inconsistent, with some errors resulting in a throw and others being passed to the callback. I'm good with passing them to the callback, it just needs to be consistent (e.g. all validation errors always going to the callback or always throwing).

I agree that they should be consistent but as long as it is not a "programmers" error I think it should land in the callback.

BridgeAR · 2017-09-20T14:18:03Z

lib/fs.js


-  if (!nullCheck(path, callback))
-    return;
+  if (typeof path !== 'string' && !Buffer.isBuffer(path)) {


We actually accept all TypedArrays right now if I am not mistaken. Did you want to limit it to buffers?

I'll double check

BridgeAR · 2017-09-20T14:20:01Z

lib/fs.js

  req.oncomplete = makeCallback(callback);
  binding.access(pathModule._makeLong(path), mode, req);
 };

-fs.accessSync = function(path, mode) {
-  handleError((path = getPathFromURL(path)));
+fs.accessSync = function(path, mode = fs.F_OK) {


As the validation of the mode is later it would be better performance wise to check for undefined (the Number.isInteger check is obsolete in that case). But this might be over optimizing stuff.

BridgeAR · 2017-09-20T14:21:23Z

lib/fs.js

@@ -329,9 +242,9 @@ Object.defineProperty(fs.exists, internalUtil.promisify.custom, {


 fs.existsSync = function(path) {
+  handleError(path = getPathFromURL(path));
+  nullCheck(path);


BridgeAR · 2017-09-20T14:27:41Z

lib/fs.js

@@ -827,117 +808,192 @@ fs.ftruncate = function(fd, len, callback) {
  if (typeof len === 'function') {
    callback = len;
    len = 0;
-  } else if (len === undefined) {
+  } else if (len == null) {


Using the explicit version is actually better performance wise. But do we really want to support using null at all? I did not check but I guess you just ported that over from the c++ layer?

yeah, this simplifies an additional check that was being done in C. There's an existing comment in the C code about the inconsistency here.

I think == null is optimized anyway

@benjamingr that is currently not correct.

See #15178.

BridgeAR · 2017-09-20T14:30:54Z

lib/fs.js

-    return;
-  if (!nullCheck(path, callback)) return;
-  var req = new FSReqWrap();
+  mode = modeNum(0o777);


The modeNum wrapping here is obsolete. I think you actually wanted to do

if (typeof mode === 'function') { callback = mode; mode = 0o777; } else { mode = modeNum(mode, 0o777); }

The same applies to other functions.

BridgeAR · 2017-09-20T14:37:38Z

lib/fs.js

+
+  if (typeof path !== 'string' && !Buffer.isBuffer(path)) {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path',
+                               ['string', 'Buffer', 'URL']);


I think it would be great if all the errors could also contain the actual value.

BridgeAR · 2017-09-20T14:50:57Z

benchmark/fs/promises-access.js

+  });
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    access(__filename, () => countdown.dec());


I am not convinced that running 2e5 calls in the same tick is actually a good way of benchmarking this. I think it would be better to either use a recursive strategy to see the individual call length or a batched recursive call that batches up to lets say 500 calls together.
This applies to all these benchmarks.

I agree, ideally if we can get a more applicative benchmark that'd be the best

In particular, the promises version always queue the callback to run later every time here which would be disproportionately represented in the above benchmark (I think)

mhdawson · 2017-09-20T16:53:18Z

@nodejs/tsc for awareness

ofrobots · 2017-09-20T18:31:25Z

nit: in the PR abstract s/promisified/promised

MylesBorins · 2017-09-20T19:00:23Z

re: fs.fastRealpath please refer to these references about age old security vulns found in realpath(3). The potential off by one sec vuln was why it wasn't built on top of originally. I landed a fix into libuv that should guard against this, but we should likely have a larger discussion about this before introducing the api

#2680 (comment)
https://www.kb.cert.org/vuls/id/743092

benjamingr · 2017-09-21T13:31:04Z

Note that these are not util.promisify wrappers. These are implementations of the fs functions with Promises without any wrapping

Can we get a benchmark comparing the two?

jasnell · 2017-09-21T15:00:52Z

@benjamingr ... there are some in the original post at the top. e.g.

james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 280,078.9048933864
fs/promises-access.js method="promisify" n=200000: 69,310.04884236863
fs/promises-access.js method="promise" n=200000: 240,928.00098102028
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 236,062.80841465757
fs/promises-access.js method="promisify" n=200000: 82,258.05538216804
fs/promises-access.js method="promise" n=200000: 192,776.68164314007
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 112,929.07886994253
fs/promises-access.js method="promisify" n=200000: 136,272.83764802708
fs/promises-access.js method="promise" n=200000: 169,612.60194223037
james@ubuntu:~/node/node$ ./node --no-warnings benchmark/fs/promises-access.js
fs/promises-access.js method="legacy" n=200000: 163,294.66290369138
fs/promises-access.js method="promisify" n=200000: 119,130.69228308211
fs/promises-access.js method="promise" n=200000: 185,855.53426089959

The second entry in each run uses util.promisify, the third entry uses the approach in this PR.

benjamingr · 2017-09-21T16:33:10Z

cc @bmeurer

btd · 2017-09-24T09:34:18Z

lib/internal/errors.js

@@ -259,7 +260,9 @@ E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
 E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
 E('ERR_OUT_OF_RANGE', 'The "%s" argument is out of range');
+E('ERR_OUTOFBOUNDS', 'The "%s" argument is out of bounds');


Maybe better ERR_OUT_OF_BOUNDS (to be consistent with prev line)?

pitaj · 2017-10-02T01:56:02Z

Why is this labelled semver-major? It doesn't appear to be a breaking change, shouldn't it be a minor version feature addition?

jasnell · 2017-10-02T01:58:34Z

It makes a number of breaking changes including changes to error handling in the existing fs apis

trevnorris · 2017-10-12T09:30:22Z

lib/fs.js

+
+  if (!Number.isInteger(len) || len < 0) {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'len',
+                               'unsigned integer');


Can't a lot of these checks be done in c++? it would be a nice performance win.

Pulled many of these out of C++ in order to get better consistency with the move to internal/errors and consistency with where the type checking is happening. As it is, the type checking is rather... inconsistent. Will look at it again, tho.

jasnell · 2017-12-18T19:40:40Z

Work continued in #17739

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 20, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 20, 2017

jasnell added fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises. labels Sep 20, 2017

addaleax reviewed Sep 20, 2017

View reviewed changes

geek reviewed Sep 20, 2017

View reviewed changes

BridgeAR reviewed Sep 20, 2017

View reviewed changes

[Squash] address some feedback

df1d587

BridgeAR reviewed Sep 20, 2017

View reviewed changes

btd reviewed Sep 24, 2017

View reviewed changes

bojand mentioned this pull request Oct 4, 2017

[NodeJS] Promises and async/await grpc/grpc-node#54

Open

trevnorris reviewed Oct 12, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

jkrems mentioned this pull request Dec 13, 2017

Support url *strings* in fs APIs #17658

Closed

jasnell mentioned this pull request Dec 18, 2017

[WIP] fs promises #17739

Closed

4 tasks

jasnell closed this Dec 18, 2017

aduh95 mentioned this pull request Mar 14, 2021

Different runtime behaviour of fs.realpath vs fs.promises.realpath is very confusing #37737

Open

fs: initial experimental promisified API #15485

fs: initial experimental promisified API #15485

Conversation

jasnell commented Sep 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

jasnell commented Sep 20, 2017

mscdex commented Sep 20, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 20, 2017

addaleax commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Sep 20, 2017

ofrobots commented Sep 20, 2017

MylesBorins commented Sep 20, 2017

benjamingr commented Sep 21, 2017

jasnell commented Sep 21, 2017

benjamingr commented Sep 21, 2017

Choose a reason for hiding this comment

pitaj commented Oct 2, 2017

jasnell commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Dec 18, 2017

jasnell commented Sep 20, 2017 •

edited

Loading