Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Support awaiting values through util.awaitable #12

Closed
benjamingr opened this issue Aug 29, 2016 · 123 comments · Fixed by nodejs/node#12442
Closed

Support awaiting values through util.awaitable #12

benjamingr opened this issue Aug 29, 2016 · 123 comments · Fixed by nodejs/node#12442

Comments

@benjamingr
Copy link
Member

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)

  • object | function - an object to convert to an awaitable API or a function taking a callback

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
Copy link
Member Author

Ping @nodejs/collaborators

@vkurchatkin
Copy link

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
Copy link
Member Author

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

@vkurchatkin
Copy link

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
Copy link
Member

ChALkeR commented Aug 29, 2016

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
Copy link
Member

ChALkeR commented Aug 29, 2016

Also /cc @petkaantonov

@benjamingr
Copy link
Member Author

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
Copy link

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
Copy link

caitp commented Aug 29, 2016

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

@littledan
Copy link

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member

gsathya commented Aug 29, 2016

@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
Copy link
Member Author

benjamingr commented Aug 29, 2016

@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
Copy link
Member

targos commented Aug 29, 2016

@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
Copy link
Member Author

@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
Copy link

domenic commented Aug 29, 2016

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
Copy link

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
Copy link

CrabDude commented Aug 29, 2016

[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
Copy link
Member Author

@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
Copy link

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

@littledan
Copy link

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
Copy link
Member Author

@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
Copy link

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

@domenic
Copy link

domenic commented Aug 30, 2016

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

@littledan
Copy link

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
Copy link

domenic commented Aug 30, 2016

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
Copy link
Member Author

@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.

@bmeck
Copy link
Member

bmeck commented Apr 26, 2017

@misterdjules @ljharb rejection maps to throw. Both would abort.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2017

yes - both would abort, or not, the same. Which is why "do the same thing as uncaught exceptions" isn't really analogous to unhandled rejected promises, even though there are some that believe it should be.

@benjamingr
Copy link
Member Author

@misterdjules can we move our post mortem discussion to a separate issue?

It's interesting to discuss but it's about promises in general rather than util.awaitable. I'll try to open one soon.

@misterdjules
Copy link

@benjamingr

I'd also like to point out we currently don't have a good solution for post-mortem debugging in production with callbacks anyway

We do have a solution that people use today and that suits their use case.

(and outside of production you can just reproduce it and look at the stack/heap with the debugger)

Post-mortem debugging has, by definition, fundamentally different characteristics than live debugging. Suggesting to use one when users need to use the other is not going to help. When I'm using post-mortem debugging, it's because using live-debugging would not have the characteristics I'm looking for, and vice-versa.

You can see nodejs/post-mortem#18 on why - Node servers not using promises have to be very careful to not be susceptible to DoS as is - and if they add try/catch in places to avoid it, you still have to lose some core dumps to keep availability up.

Some systems can use post-mortem debugging and have the same (or better) availability than similar systems not using post-mortem debugging.

However, using post-mortem debugging can be challenging and can require modifications to the design of the system that is debugged. These systems often need to be designed explicitly to be "crash only". One aspect of that work is choosing which components need to crash on which errors.

In other words, pointing at an example where crashing on errors would bring availability down does not necessarily invalidate that methodology, it often means that the system used as an example needs to be designed differently if post-mortem debugging is to be used.

@benjamingr
Copy link
Member Author

@misterdjules I'm opening a new issue to address postmortem issues - I'd rather this one focus on util.awaitable since it has been established several times now that this method does not add a promise core to Node and does not invalidate any of your postmortem concerns :)

@misterdjules
Copy link

@jasnell

The specific opt-in on this is what makes it the most attractive.

It is opt-in in the sense that users are free to use it or not in their code, but it is a bit more subtle when we consider that a significant number of node programs use third party modules. In this case, a third party module opting into using util.promisify cannot be controlled by the program that uses that module, sometimes indirectly.

It's easily possible to blacklist the use of Promises in an application and the existing APIs continue to work as they have always done.

In the context of a program using third party modules that opt into using promises, blacklisting promises could make the program non functional, so it doesn't seem to be a good solution to that problem.

@benjamingr
Copy link
Member Author

In the context of a program using third party modules that opt into using promises, blacklisting promises could make the program non functional, so it doesn't seem to be a good solution to that problem.

That's kind of your point though, isn't it @misterdjules ? A third party module could use promises (or just defer errors to avoid Zalgo) anyway. If you want to blacklist promises you're still stuck with the millions of packages using third party promises, and no one is stopping anyone from rolling promises (in ~100 LoC) into their own app without declaring a dependency, or just using the new Promise constructor in V8, or even an async function.

You have no way to blacklist promises today anyway without an AST stage that detects the async keyword and blacklist based on that. util.promisify has no effect on the amount of ability you have on that.

@misterdjules
Copy link

@mikeal

Some of these comments seem to dislike the error model Promises have. While that's a fine opinion to have it has become an incredibly tired argument against anything Promises related in Node.js Core. Core doesn't get to decide when/if people use Promises, they already are. It's the responsibility of Core to improve the experience of those users in Node.js. Blocking any improvements to that experience does nothing to slow down Promise adoption and only makes Node.js worse for a growing set of our users.

I wish I could use promises, but I'm presenting a use case where I can't, and using code that does use promises would break the systems that I operate.

I'm also trying to represent some users of node, some of them prominent, and their use cases. As a member of the post-mortem working group, it seems it is the least I can do.

I'm expressing concerns and opposition to a change, which as far as I know is allowed and encouraged by the project.

In the past and still today I have also tried to be constructive and work towards trying to find a solution to these concerns. See https://gist.github.com/misterdjules/2969aa1b5e6440a7e401, https://gist.github.com/misterdjules/30db8fc651746c6f917a and https://gist.github.com/misterdjules/d7674cf6c42149552958 for examples of how I spent a significant amount of time on exploring solutions .

If the project wants to move ahead with util.promisify, and if I'm still opposed for the reasons I have presented, the project can vote and merge that change if the vote passes.

In the meantime, I'm happy to respond to questions or help in any way I can.

So I'm not sure how this is blocking anything.

@benjamingr
Copy link
Member Author

benjamingr commented Apr 26, 2017

@misterdjules I've read those files last year, I'd like to see how we can move forward and I've proposed some alternatives in #114 including your attempts to bend the V8 implementation to break the spec.

My point here about post-mortem debugging is that while this change is (IMO) a big help to promise users for the reasons outlined above - it leaves post-mortem debugging (which is broken for promises anyway) exactly as expressive as it was before - without adding a core promises API.

Your points here are welcome, and in case I haven't made it clear - I welcome and appreciate your feedback and look forward to finally fixing this problem together. I would however prefer we could discuss the post-mortem issue in this forum in a way that focuses on it in #114 . The discussion we're having makes it hard for people to discuss other concerns (API or other) with util.promisify.

@misterdjules
Copy link

The discussion we're having makes it hard for people to discuss other concerns (API or other) with util.promisify.

Sounds good! I will not comment further about issues specific to post-mortem debugging here.

@seishun
Copy link

seishun commented Apr 28, 2017

I've been meaning to post my little idea regarding promises in core APIs. Sorry if this is the wrong place, or it has been discussed before. In short:

require('fs') returns callback-based API as before.
import "fs" returns promise-based API.

Personally, I'm not a huge fan of promises, but if they are (or are going to become) the de-facto standard in the JS ecosystem, it will be quite sad if many years from now people using Node.js will still have to explicitly opt-in to use them. It seems ES6 modules is a chance to make them the "default" without breaking backward compatibility.

We might also want to introduce some mechanism to get the callback-based API with import (or vice versa), e.g. import "fs/callback". But that's beside the point - the main idea is that people can use promise-based API without any special magic like util.awaitable or fs/promise or fs.readFile.promise.

@styfle
Copy link
Member

styfle commented Apr 28, 2017

@seishun I like the idea because its forward thinking. But there are a lot of people who already write their code using ES6 imports and transpile it to CommonJS. So this would prevent them from just disabling the transpiling step and instead force them to actually change the logic of their code since it means something different.

@MadaraUchiha
Copy link

What about a --legacy-callback-api flag that passes all require()d native modules through util.toNodeback() or some similar thing?

@mcollina
Copy link
Member

I think discussing promisifying all of core is out of scope for this issue.

I think the current approach is extremely sensible. +1 for the proposed approach, as it improves things for Promise users.

Post-mortem debugging is still problematic with Promises, and I think is a very very useful technique. The current approach makes Promises opt-in.

addaleax added a commit to addaleax/node that referenced this issue May 9, 2017
Add `util.promisify(function)` for creating promisified functions.
Includes documentation and tests.

Fixes: nodejs/CTC#12
PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
Add `util.promisify(function)` for creating promisified functions.
Includes documentation and tests.

Fixes: nodejs/CTC#12
PR-URL: nodejs#12442
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: William Kapke <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.