nodejs / CTC

Node.js Core Technical Committee & Collaborators
80 stars 27 forks source link

Support awaiting values through `util.awaitable` #12

Closed benjamingr closed 7 years ago

benjamingr commented 8 years ago

Adding actual promise support to Node has proven really difficult and a tremendous amount of work. In addition post-mortem concerns and other concerns proved to be a big burden. I think given the very large amount of work and the fact that the topic is highly debateable we should instead go with a simpler approach. async/await recently landed in v8.

I think we need a solution that gives users a version of the Node API that works with async/await but does not require us to touch the entire API piece by piece. I suggest we expose a util.awaitable method that takes a standard Node callback API and converts it to be consumable with async/await.

For example:

const fs = util.awaitable(require(fs));
async function foo() {
  const result = await fs.readFile("hello.txt"); // file contains "World"
  console.log("Hello", result);
}
foo(); // logs `Hello World`

util.awaitable(object | function)

If this method is passed an object, the method returns an object with the same method names -each of the methods returns an awaitable value (promise) instead of taking a callback.

If this method is passed a function, the method returns a function that does the same thing except it returns an awaitable value (promise).

Prior work

Bluebird has ~10 million monthly downloads on NPM and it provides a Promise.promisifyAll function which is very widely used. In addition other promise libraries like Q (7m downloads) also provide a similar function.

I suggested this solution back in 2014 but in 2014 we didn't have async/await finalized in the spec, v8 promises were slow and there was a much weaker case for adding it.

Why in core?

Promises and async/await are a language feature. It is impossible to implement util.awaitable without v8 private symbols so it can't be done in userland. The only way to do it fast from userland is to use a promise library like bluebird.


Basically I'm looking for some discussion around this. I feel like this is a much much simpler in terms of code added and maintenance load than actually changing the entire API to support promises which is the prior solution the CTC discussed and was in favor of.

Some "Promise people " might not be too thrilled about it - I'm not. I do however think it's a very reasonable compromise.

benjamingr commented 8 years ago

Ping @nodejs/collaborators

vkurchatkin commented 8 years ago

It is impossible to implement util.awaitable without v8 private symbols so it can't be done in userland

What do you mean? It's totally possible

benjamingr commented 8 years ago

@vkurchatkin not without using the promise constructor which allocates a closure object - unless there is something I'm not aware of.

vkurchatkin commented 8 years ago

not without using the promise constructor which allocates a closure object

The main thing is: it's possible. Allocation a closure is just a detail

ChALkeR commented 8 years ago

This doesn't look like the best approach to provide a Promises-based api, but we might want a function like that even if we would have a normal Promises-based api, because a lot of modules were written using errbacks.

Re: closure and possibility to implement this as a thirdparty module in pure js — @benjamingr, what are performance implications here for implementing it on c++ side vs js-side impl that constructs a new Promise? I assume that the latter could be considerably slower, but I still would like to see the exact benchmark numbers for that.

ChALkeR commented 8 years ago

Also /cc @petkaantonov

benjamingr commented 8 years ago

The main thing is: it's possible. Allocation a closure is just a detail

It's impossible to write code that does this in a performant way without a core method.

Re: closure and possibility to implement this as a thirdparty module in pure js — @benjamingr, what are performance implications here for implementing it on c++ side vs js-side impl that constructs a new Promise? I assume that the latter could be considerably slower, but I still would like to see the exact benchmark numbers for that.

I think Petka wrote a benchmark for that at one point - V8 uses private properties all the time for creating resolved promises. I can try to work on a benchmark if we're interested in a util.promisify under the assumption that if it's considerably faster then we want it.

I'd like to ping in @littledan @ajklein and @caitp to perhaps weigh in from the v8 side on the promise implementation in V8 since it was refactored not too long ago.

vkurchatkin commented 8 years ago

It's impossible to write code that does this in a performant way without a core method

Please, define "performant". It is good enough for me, for example.

caitp commented 8 years ago

the Promise implementation in V8 is (likely) to undergo more serious refactoring in the near future. @gsathya may have details

littledan commented 8 years ago

If you use the V8 C++ API, you can allocate a Promise there and the current implementation will not use resolve/reject closures. This could be done from core or a native module. However, lots of closures are created anyway currently in the promise execution code path, and as @caitp said, we are looking into changes here.

If Promise performance is an issue for Node in real-world code, what would be more useful than elaborate workarounds like what is suggested here is benchmarks that we could use to try to optimize Promises against. @benjamingr, have you observed slowdowns from using promisify?

Note that async/await is still behind the --harmony flag.

benjamingr commented 8 years ago

@littledan thanks for weighing in. I have not benchmarked native promises recently but I recall a few months ago that I benchmarked code converting callback APIs to promises with native promises and bluebird and I noticed that the closure approach (using the promise constructor) is significantly slower than the approach bluebird takes (not using a closure).

It's entirely possible that the recent refactoring of promises changed this but I have no reason to believe so. This is a penalty that every single asynchronous call incurs in Node - they add up.

benjamingr commented 8 years ago

By the way - @littledan I know you already use the bluebird benchmarks (from the blog). They're the ones I trust the most at this point in time as they simulate a real use case. Native promises are spending quite a bit of time there in "promsifying" and I suspect part of the edge bluebird has over native promises at this point in time is because of that.

gsathya commented 8 years ago

@benjamingr Can you share the code that you used to benchmark this? Promisify seems to be a bluebird specific function, so v8 wouldn't be able to support or optimize anything non standard, but I'm happy to look at code that seems to be slow because of Promises.

benjamingr commented 8 years ago

@gsathya I'm running the bluebird benchmarks - http://bluebirdjs.com/docs/benchmarks.html namely, bluebird can resolve without a closure so it can convert functions taking callbacks to functions returning promises. From the Chromium blog I recall this is the benchmark you're running too.

Bluebird does this: https://github.com/petkaantonov/bluebird/blob/master/src/promisify.js#L124-L214 , namely it creates a new function for the original and resolves it without a closure (https://github.com/petkaantonov/bluebird/blob/master/src/nodeback.js#L42) since it can create a Promise without passing an executor.

V8 promises follow a similar design where passing a special (unexposed) symbol allows one to create a promise without passing an executor inside and then resolving it manually via .resolve on the NewPromiseCapability. This for example is done in your implementation of Promise.all.

Because userland code cannot do this - calling promisified functions is slower for native promises in the benchmark because it is done via a closure.

This proposal can be accomplished by implementing util.awaitable with a NewPromiseCapability rather than a new Promise which would make it fast,

targos commented 8 years ago

@littledan @benjamingr. V8 extras have access to special functions to create and resolve/reject promises in JS without closures. Would it make sense / is it recommended to go down that road ?

benjamingr commented 8 years ago

@targos yes definitely. It would let us write this entirely in JS as long as we're in core - I don't have any experience with v8 extras.

Also going to ping @domenic so he is aware of this discussion although I'm not sure he would be interested in participating - welcoming his input regardless.

domenic commented 8 years ago

The problem is bridging the gap between Node's module system and V8 extras. I think @bnoordhuis has previously looked into this but I don't recall his conclusions.

In this case where things are very simple, there are a few easy solutions though. You could use V8 extras' binding object and access that from C++ (and then expose it to JS via Node's internal modules or process.binding() or similar). Or you could even just set a global property, and grab it and delete it during startup.

In general I'd agree that benchmarks would be more interesting as I think focusing on the one or two closures here is not going to get you as big gains as letting the V8 team work their magic in other places. So if someone wanted to whip up a sample app that uses promises by naively promisifying lots of core APIs and then compare that to Bluebird which uses closure-less promisification, that would be pretty helpful. Maybe doxbee and friends are that already, but in that case I'm curious to hear @gsathya's perspective on whether the remaining gaps between Bluebird and native are due to the closures or due to other factors.

chrisdickinson commented 8 years ago

In general I'd agree that benchmarks would be more interesting as I think focusing on the one or two closures here is not going to get you as big gains as letting the V8 team work their magic in other places. So if someone wanted to whip up a sample app that uses promises by naively promisifying lots of core APIs and then compare that to Bluebird which uses closure-less promisification, that would be pretty helpful. Maybe doxbee and friends are that already, but in that case I'm curious to hear @gsathya's perspective on whether the remaining gaps between Bluebird and native are due to the closures or due to other factors.

FWIW, it wouldn't take much work to get nojs to a point that we could get timings for native V8 promise bindings direct to libuv — at least a few of the fs bindings are already available.

CrabDude commented 8 years ago

[Relocated from #7]

util.awaitable seems like a simple, easy and attractive solution for several reasons:

  1. There is merit in its existence independent of any eventual "promises in core" and is thus worth supporting indefinitely compared with alternative short-term solutions.
  2. Core can assure proper coordination with global.Promise and unhandledRejection for userland to leverage.
  3. util.awaitable would serve as a stepping stone toward a more complete solution (i.e., import {promise as fs} from 'fs/promise'), allowing for quirks, edge-cases, performance, tooling, etc... consideration to be addressed now in anticipation.

Should util.awaitable use global.Promise (allowing a userland extension point) or cache / integrate more deeply with V8 Promises for performance or predictability? (For me, the former is de facto and more JS-y)

Nested methods would be a consideration, since they will not be promisifiable w/o app-developers understanding JS' object model, a Object.prototype.promise method or coupling util.awaitable to core APIs:

let fs = util.awaitable(require('fs'))
let promise = fs.foo.bar('asdf') // Fail. promise === undefined

Also, FWIW, util.awaitable requires an extra line when used with import and may not play nice with flow or Typescript (not critical, but a pro for _#_3 above):

import fs from 'fs'
fs = util.awaitable(fs) 
// vs
import {promise as fs} from 'fs'
benjamingr commented 8 years ago

@CrabDude I agree with everything you said here. I also think it's a very attractive solutions, I agree with the drawbacks and that given the circumstance it's the best solution we can do in the meantime.

vkurchatkin commented 8 years ago

I'd say that at this point there is no evidence that having something like this in core would be beneficial

littledan commented 8 years ago

What coordination will core be able to do that user libraries will not be able to do? I don't know of any deeper ways that core could access Promise internals.

benjamingr commented 8 years ago

@littledan

V8 promises follow a similar design where passing a special (unexposed) symbol allows one to create a promise without passing an executor inside and then resolving it manually via .resolve on the NewPromiseCapability. This for example is done in your implementation of Promise.all.

littledan commented 8 years ago

How do you intend to get access to this symbol from core?

domenic commented 8 years ago

Per upthread discussions, using either the C++ API or V8 extras would suffice.

littledan commented 8 years ago

Right, but there's no direct access. In general, my understanding is that core and user libraries have access to the same V8 API; is that wrong?

domenic commented 8 years ago

Yeah, that's not quite right. Core can use C++ without making the consumer take on the burden of installing a compiler toolchain. And core can use V8 extras since it is part of the bootstrap process, whereas user libraries cannot.

benjamingr commented 8 years ago

@littledan in general, a lot of things that are done in core can be done from userland through a native C++ module. I don't think requiring users to install an additional native C++ module or swap the promises implementation altogether is a good idea - language features should "just work" with the node APIs.

I think most people agree that Node APIs need a variant that works with async/await in the future - this is a stopgap.

littledan commented 8 years ago

OK, no objection from me, just curious. I didn't understand the disadvantages of native modules for this sort of thing. Thanks for explaining.

bnoordhuis commented 8 years ago

I think most people agree that Node APIs need a variant that works with async/await in the future - this is a stopgap.

The problem with stopgaps in core is that they have to be maintained for a long time, possibly forever.

I don't see a problem with a C++ add-on. It can ship as precompiled binaries to remove the toolchain dependency and could live under the nodejs umbrella if people think official-ness is important.

benjamingr commented 8 years ago

I don't see a problem with a C++ add-on. It can ship as precompiled binaries to remove the toolchain dependency and could live under the nodejs umbrella if people think official-ness is important.

I think its officialness is very important, and honestly as something we can add to util as a 20 line fix that will give people a surefire way to use the async/await language feature with callbacks it's definitely worth our while.

I don't think this is a big burden to maintain forever and it's something that's really useful to users even outside of core APIs and a capability users can't have with an API.

The idea of util.awaitable vs a promise core is that it's hopefully a lot less objectionable, it poses a lot less controversy, it's a lot less opinionated and it means exposing a capability that users can build on and not an API.

gsathya commented 8 years ago

Bluebird does this: https://github.com/petkaantonov/bluebird/blob/master/src/promisify.js#L124-L214 , namely it creates a new function for the original and resolves it without a closure (https://github.com/petkaantonov/bluebird/blob/master/src/nodeback.js#L42) since it can create a Promise without passing an executor.

V8 promises follow a similar design where passing a special (unexposed) symbol allows one to create a promise without passing an executor inside and then resolving it manually via .resolve on the NewPromiseCapability. This for example is done in your implementation of Promise.all.

Bluebird "wins" here because it doesn't need to create the resolve and reject closures to pass to the executor. It has a specialized promisify API that lets it reach into the promise object and attach resolve/reject callbacks and call its internal fulfill method directly.

This will not be possible with native promises even with exposing the special symbol, because we always have to create the resolve/reject closures as the internal callbacks/fulfill method aren't exposed. The resolve called in Promise.all is such a closure.

ChALkeR commented 8 years ago

@benjamingr +1 to this if it will give any measurable benefits compared to a js-side user function that would just wrap methods in new Promise (using native promises). If so, I would prefer this to get into v7.

benjamingr commented 7 years ago

@caitp how hard would it be to implement this in v8 promises efficiently? (without allocating an extra closure - like new Promise)

caitp commented 7 years ago

In the current implementation upstream, I don't think it would be too difficult to do what you want, And we could probably even implement it upstream and expose it to Node via v8extras (reason to have it upstream in v8: reading/writing internal Promise fields not possible via js).

benjamingr commented 7 years ago

@caitp that would be amazing! Happy new year!

haraldrudell commented 7 years ago

I think this proposal is far too narrow. and we should use import, not require.

async functions is the opportunity to think big about the Node.js api and a second chance on life

  1. async is basically 70% less code, something similar less bugs, so callback going forward should be used by nothing and no-one: put that whole thing in maintenance.

  2. Define the Node.js api we want, possibly very similar to what we already have, but an opportunity to scrap obsoletes, clean up those fs apis from the 90’s, eradicate uncatchable exceptions and create anything we want for our future of jet packs and flying cars. Design the api we want 3 years from now without compromise. If libuv no have it, code around it in ES meanwhile. Scrimping and short-changing our future would no be right.

    How maintainable and shortened can we make the future code written by users of Node.js?

  3. This will be needed the day Node 8 gets import (in V8 since 2017-01) when babel and callbacks can start retiring. That’s soon.

What we should be doing right now is redlining the official list of Node.js async api functions

and there should be a nodeasync github repo for the code

haraldrudell commented 7 years ago

Today, loading the Node.js api is split over some 30+ requires.

We’re smarter now and have the powers of ECMAScript 2017. It would be nice if this was a syntactic one-liner, possibly backed by lazy-loading. We no writing Java no more. Destructuring.

haraldrudell commented 7 years ago

Isn’t the solution to try-catchless callbacks to have them invoke an async function? Which means some clever way of implementing the .catch by the ES layer

For example the request event of http.Server. The need is to invoke an async request function (request, response) with proper this value and a clever implicit .catch. If we don’t have this, callbacks need to be wrapped with wasteful boilerplate. Perhaps the solution is extending an async Server class.

This implicit catch could also be the solution to the callback errors that today throw uncatchable exceptions.

haraldrudell commented 7 years ago

It’s clear enough what needs to be done: create an importable repo and let the world pull-request it.

phaux commented 7 years ago

@haraldrudell it would be a nice opportunity to replace node streams with WHATWG streams and EventEmitters with Observables 👍

benjamingr commented 7 years ago

Please open separate issues if you wish to discuss separate ideas. Work is going on for Node streams coerce into/from async iterators (observables are push streams) and observable work is still at stage 1 (we'll have to wait and see where the spec lands).

You're welcome to participate in the process - this is not the correct place. The arguments for util.awaitable are not to replace a promised core - it's to allow smooth interop with callback libraries.

benjamingr commented 7 years ago

@caitp any news :)?

addaleax commented 7 years ago

@benjamingr I’ve recently been playing around with this a bit in

One thing I realized is that we might want to wait for async-hooks to land (PR) before doing this, to get some proper MakeCallback-esque way of resolving promises; I have some ideas for that but didn’t implement that. That’s one of the reasons I’m trying to push the PR, though.

benjamingr commented 7 years ago

@addaleax that's awesome :)

I'm in favor of landing a util.awaitable as soon as possible (even in 8.0) and then optimizing it later.

addaleax commented 7 years ago

@benjamingr Okay, can you take a look at the commits there and tell me if you think it’s PR-ready (apart from the lack of tests and docs :smile:)?

benjamingr commented 7 years ago

Sure, I'll leave comments in - I can work on docs and tests on Tuesday and PR that against your branch if that would help promote this.

Left comments on https://github.com/addaleax/node/commit/fbec98b03a57fb47ff736ec92073db748f871a74

Also, could you point me to the internal bindings (promisify without a closure)? I couldn't figure out where this is in https://github.com/addaleax/node/commit/8bd26d3aea0c3964881bfbd045080abea3f71012

addaleax commented 7 years ago

Also, could you point me to the internal bindings (promisify without a closure)? I couldn't figure out where this is in addaleax/node@8bd26d3

Ooops, wrong copy&paste. addaleax/node@f59507b34d2f1ec439a4e0eb2feed72baa8162a6 :)

caitp commented 7 years ago

@caitp any news :)?

been focused on implementing the async iteration proposal, so I haven't done any work to put any of this in v8-extras.

But I would say start with an embedder-implemented version, and if there are pain points identified that can only be addressed in v8, then we can look at doing the work on the v8 side. +1 @addaleax

addaleax commented 7 years ago

PR: https://github.com/nodejs/node/pull/12442