[WIP] fs promises by jasnell · Pull Request #17739 · nodejs/node

jasnell · 2017-12-18T19:39:29Z

This is a work in progress continuation of the prototyping work started originally in #15485.

Ping @MylesBorins @addaleax @mcollina

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 · 2017-12-18T19:46:11Z

lib/fs.js

This change is necessary because using an async function for stat operations means we have a race condition on the use of statValues when there's just a single TypedArray. So, for the normal callback case, we keep the single shared typed array, but for the promises version, we allocate one TypedArray per Promise. It's a bit more expensive for the promises case, but keeps things status quo for the callback case.

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

lib/fs.js

@mcollina ... take note of this function. It is an async generator used in an async iterator within fs.promises.readFile(). Significantly less complicated than the current callback based implementation.

targos · 2017-12-18T20:28:43Z

lib/fs.js

there is no place where loop is incremented

oh, that's a left over... should have removed that.

targos · 2017-12-18T20:35:16Z

lib/fs.js

shouldn't this be a return?

targos · 2017-12-18T20:41:15Z

lib/fs.js

I'm afraid it's bad for performance to concat for each chunk. Is it possible to push to an array and concat once we are out of the loop?

yep, whatever works best.

for await is very slow at the moment

yep... part of my goal is to make this available then see what @bmeurer and team can do in terms of making it faster.

@jasnell this would make the promise API prohibitebly expensive to use though.

While the API is still experimental, performance is not the priority. If we're not able to get the performance optimized adequately, then we can move to a different approach.

addaleax · 2017-12-18T23:35:43Z

lib/fs.js

I’d drop the , position if it isn’t necessary… without it, this should work on un-seekable files (e.g. TTYs, pipes) as well?

Yeah, had that on my list to work through

addaleax · 2017-12-18T23:37:16Z

lib/fs.js

Can you set fs.access[util.promisify.custom] = fs.promises.access; etc.?

Yep. I will be

addaleax · 2017-12-18T23:42:35Z

src/node_file.cc

To keep myself from doing silly things until it's a bit more baked.

addaleax · 2017-12-18T23:44:48Z

src/node_file.cc

sidenote, using &req->statbuf will let you get rid of the cast + remove one layer of indirection

addaleax · 2017-12-18T23:46:20Z

src/node_file.h

Can you make this enum class? I think that would be a bit more readable since COPY and MOVE are pretty short identifiers and it’s not obvious what kind of thing they refer to…

Also good call

starkwang · 2017-12-19T02:28:48Z

lib/fs.js

Using ERR_INVALID_FD might be more suitable for the errors like this.

mcollina · 2017-12-21T08:03:20Z

lib/fs.js

+      eof = true;
+    yield buffer.slice(0, length);
+  }
+}


This is going to be very slow. Do we know if there is some improvements in the area from the V8 team?
Can you use the same class approach I used for Async Iterators? It ended up being 2x the async generator function.

Yes this can use the class approach, but I'm purposefully leaving this in so that folks my @bmeurer can take a look and figure out how to make it faster. I will be switching it to the class approach a bit later.

mcollina · 2017-12-21T08:05:42Z

lib/fs.js

+    length -= written;
+    if (position !== null)
+      position += written;
+    return writeEverything(fd, buffer, offset, length, position);


If the buffer is big enough, it could lead to a stack overflow. Can this be done as a loop?

Given that it returns a Promise, does it overflow the stack? :-)

That said, I'm already planning to rework this function. This was a temporary impl

jasnell · 2017-12-21T18:48:07Z

@bmeurer / @fhinkel ... question for you.

With this implementation currently... within an async function, I create an instance of FSReqPromise, which is a handle object provided by process.binding('fs'). On construction, it creates a V8 Promise at the C++ layer. The async function then calls back down to C++, passing the FSReqPromise along with it. At the C++ layer, we detect that this handle thing is an FSReqPromise and instead of calling a callback, we resolve the Promise when the operation completes.

It works rather nicely as is, however... it is a bit inefficient because we already know that we are running within an async function, which we know will create it's own Promise. What I would like to be able to do is avoid creating the FSReqPromise at all and have it so that the process.binding('fs') exposed functions know they are executing within an Async Function and give them the ability to access the Promise that is created.

This would save two additional allocations (one for the FSReqPromise handle and one for the v8::Promise it creates) and would eliminate one boundary cross per operation.

Thing is, I don't know if this would be possible within V8 (I know it's not possible now).

For instance.... something like:

static void Open(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);
  Local<Promise> promise - args.Promise();
  ...
  args.GetReturnValue().Set(promise);
}

addaleax · 2017-12-21T18:51:16Z

@jasnell I think that would basically be equivalent to having native async functions, right? I can’t tell you whether V8 wants it or not, but it should certainly be doable…?

jasnell · 2017-12-21T18:52:36Z

Yes, that's exactly it. I want the native code to be able to know that it's running within an async function() without us having to guess and without us having to pass in additional context that wastes allocations.

jasnell · 2017-12-21T19:09:04Z

Or, perhaps a different approach... having the ability to explicitly create Async Functions at the native level with an AsyncFunctionCallback/AsyncFunctionCallbackInfo construct... e.g.

void AsyncThing(const AsyncFunctionCallbackInfo<Value>& args) {
  Local<Promise> promise = args.Promise();
  // resolve sync or async here... setting the return value like below would resolve sync
  args.GetReturnValue().Set(1);
}

such that the function itself returns a Promise and the args.GetReturnValue().Set(1) resolves the promise, and any exceptions thrown on the isolate from within the function trigger a rejection.

However it works, I just want to avoid having to allocate the FSReqPromise handle and the additional Promise.

bmeurer · 2017-12-21T19:40:28Z

@jasnell That does sound possible. I don't see why async API functions shouldn't be supported, but I'm not the expert here.

Let me redirect this question to @hashseed, @caitp, @schuay and @gsathya. WDYT?

gsathya · 2017-12-22T07:55:14Z

Oh, exciting to see all this progress! Nice work @jasnell 😄

Async API functions can be supported but I don't see the need here. Looking at methods in promises -- almost all the async methods can just be normal methods. They don't use await, they just return a promise, so there's no need for them to be async.

A couple of them do use await in that case you don't want to use the promise created by the async function because that could mess with the behavior/timing of the async function.

On construction, it creates a V8 Promise at the C++ layer. The async function then calls back down to C++, passing the FSReqPromise along with it.

The double call out to C++ can be avoided by having the binding create and return the promise. This would improve performance greatly. For example:

Instead of doing this:

    const req = new FSReqPromise();
    binding.futimes(fd, atime, mtime, req);
    return req.promise;

we could do:

    const req = binding.futimes(fd, atime, mtime);
    return req.promise;

With this change, there's only one call out to C++.

jasnell · 2017-12-22T15:27:25Z

almost all the async methods can just be normal methods. They don't use await, they just return a promise, so there's no need for them to be async.

Yeah, I've been going back and forth on this a bit. One of the key reasons they are async functions now is to simplify the error handling. The nullCheck() function, for instance, will throw an exception which is automatically handled as a promise rejection within the async function. Making those regular functions means we have to do a slight amount more work with those (obviously it's fairly trivial but still.)

you don't want to use the promise created by the async function ...

Yeah, as I was digging in to it more I realized that also. It would still be good to have the ability to create an async function at the native layer so that I can do something like:

void AsyncThing(const AsyncFunctionCallbackInfo<Value>& args) {
  args.GetReturnValue().Set(123);
}

At the C++ level and...

const binding = process.binding('foo');
await binding.asyncThing();

At the JavaScript level without having to worry about the explicit Promise creation stuff within C++.

With this change, there's only one call out to C++

Yep, and I that's ultimately where I'm going to go. The reason I've gone with the FSReqPromise model is that it is the least disruptive to the current model used by the code. The handle object is used to provide async context and continuation local storage in some cases. For instance, in some cases, preserving references to a Buffer to keep it from being garbage collected until the async operation completes. Changing the binding to return a Promise if the handle is not provided is certainly quite doable but changes some of the assumptions and requires some larger changes elsewhere. Definitely not impossible by any stretch of the imagination and likely the right way to go.

caitp · 2017-12-22T15:59:24Z

How would you model the “resume” strategy in C++?

jasnell · 2017-12-22T16:11:51Z

How would you model the "resume" strategy in C++

TBD, I think. I'm not yet 100% sure.

benjamingr · 2017-12-24T08:19:48Z

@caitp C# models them (effectively) with a class (rather than a function) with a state variable and an entry hook.

class MyAsyncFunction : AsyncFunction { // preferably, not with inheritance though
  // actually set automatically by superclass constructor, can be Finished, Thrown (?)
  int state = V8::AsyncFunction::Initial; 
  v8::Value onEnter() {
    // state is the entry number, can also use any
    switch (callCount) { // what are we awaiting?
      case 0: {
          // initial code + return
      }
      case 1: {
         // second call, and so on
      }
    }
  }
};

However I simply don't see any benefit to this over using a regular function and doing this yourself.

boost coroutines (if were used) could provide a more elegant solution - but let's don't :)

jasnell · 2018-01-22T20:09:03Z

Closing in favor of #18297

jasnell added experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. wip Issues and PRs that are still a work in progress. promises Issues and PRs related to ECMAScript promises. labels Dec 18, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 18, 2017

jasnell added dont-land-on-v4.x labels Dec 18, 2017

jasnell mentioned this pull request Dec 18, 2017

fs: initial experimental promisified API #15485

Closed

4 tasks

jasnell commented Dec 18, 2017

View reviewed changes

targos reviewed Dec 18, 2017

View reviewed changes

addaleax reviewed Dec 18, 2017

View reviewed changes

starkwang reviewed Dec 19, 2017

View reviewed changes

lib/fs.js Outdated

Copy link

Contributor

starkwang Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ERR_INVALID_FD might be more suitable for the errors like this.

jasnell added 3 commits December 19, 2017 13:52

fs: further refactoring of FSReqWrap

bcd2b8b

fs: implement fs.promises

71c3905

fs: emit experimental warning for fs.promises

21f59ec

jasnell force-pushed the fs-refactor-2 branch from 78ed377 to 21f59ec Compare December 19, 2017 22:29

mcollina reviewed Dec 21, 2017

View reviewed changes

jasnell mentioned this pull request Dec 28, 2017

assert: async parameter for assert.throws() and assert.doesNotThrow() #17843

Closed

4 tasks

This was referenced Jan 11, 2018

[WIP] fs: add FD object to manage file descriptors #18109

Closed

fs: encapsulate FSReqWrap more #18112

Closed

jasnell mentioned this pull request Jan 22, 2018

fs: add promises API #18297

Closed

4 tasks

jasnell closed this Jan 22, 2018

Uh oh!

Conversation

jasnell commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeurer commented Dec 21, 2017

Uh oh!

gsathya commented Dec 22, 2017

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

caitp commented Dec 22, 2017

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

benjamingr commented Dec 24, 2017 • edited by tniessen Loading Uh oh! There was an error while loading. Please reload this page.

jasnell commented Dec 18, 2017 •

edited

Loading

jasnell commented Dec 21, 2017 •

edited

Loading

jasnell commented Dec 21, 2017 •

edited

Loading

benjamingr commented Dec 24, 2017 •

edited by tniessen

Loading