Support awaiting values through `util.awaitable`
Closed this issue · 123 comments
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.
Ping @nodejs/collaborators
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
@vkurchatkin not without using the promise constructor which allocates a closure object - unless there is something I'm not aware of.
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
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.
Also /cc @petkaantonov
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.
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.
the Promise implementation in V8 is (likely) to undergo more serious refactoring in the near future. @gsathya may have details
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.
@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.
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.
@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.
@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,
@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 ?
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.
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.
[Relocated from #7]
util.awaitable seems like a simple, easy and attractive solution for several reasons:
- 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.
- Core can assure proper coordination with
global.PromiseandunhandledRejectionfor userland to leverage. util.awaitablewould 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 === undefinedAlso, 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'@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.
I'd say that at this point there is no evidence that having something like this in core would be beneficial
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.
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
.resolveon theNewPromiseCapability. This for example is done in your implementation ofPromise.all.
How do you intend to get access to this symbol from core?
Per upthread discussions, using either the C++ API or V8 extras would suffice.
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?
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.
@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.
OK, no objection from me, just curious. I didn't understand the disadvantages of native modules for this sort of thing. Thanks for explaining.
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.
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.
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.
@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.
@caitp how hard would it be to implement this in v8 promises efficiently? (without allocating an extra closure - like new Promise)
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).
@caitp that would be amazing! Happy new year!
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
-
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.
-
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?
-
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
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.
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.
It’s clear enough what needs to be done: create an importable repo and let the world pull-request it.
@haraldrudell it would be a nice opportunity to replace node streams with WHATWG streams and EventEmitters with Observables 👍
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.
@caitp any news :)?
@benjamingr I’ve recently been playing around with this a bit in
- addaleax/node@f59507b internal bindings for promise creation without closures
- addaleax/node@fbec98b
util.promisifyin the style of theutil.awaitabledetailed here - addaleax/node@48786be POC implementation for the
timersmodule
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.
@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.
@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 😄)?
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 addaleax/node@fbec98b
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
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@f59507b :)
@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
@misterdjules Moving conversation from nodejs/node#12442 (comment) here:
it’s just a fact that Promises are seeing widespread adoption, whether we explicitly support them in core or not (that we ship async/await now might be much more of a factor than this PR could be), so I personally don’t think it’s sensible to link this PR to the issues with debugging.
I don't understand that part of your response. Do you mean that the popularity of promises outweighs the issues they present with regards to existing best practices for error handling, including those that allow for using post-mortem debugging techniques?
What I meant is that Promise adoption happens and will continue to increase for the foreseeable future, whether we provide a direct API for them in core or not. And conversely, organizations with a no-Promises policy obviously won’t be forced to migrate off that in any case.
Adding this to core is about enabling users, and moving towards a standard way of accessing core’s APIs in a format that is already in widespread use.
Or, put another way: Right now, if people want to, they already use Promises. Adding promises support to core makes their lives, and the lives of people wanting to learn Node.js, easier, though.
Asking users to move to using async hooks seems like it could be highly disruptive for them.
One of the ideas that has been floated around was that once async_hooks is primetime-ready, domains could reasonably be re-implemented on top of them (It’s something that I think I’d really like to look into once #11883 lands). I’m not suggesting that moving from domains to bare async hooks would be easy in any way.
So I would think that if hooking promises to domain is not a large undertaking, it would be a good idea to at least investigate what it would take.
Not much, it seems: nodejs/node#12489
@addaleax going to try giving getting some volunteers to look at the PR now and see if we can add some tests :)
It’s something that I think I’d really like to look into once #11883 lands
Worries me for same reasons as stated in https://github.com/domenic/zones issues. If the intent here is a gateway to re-implement domains, I would like it to be stated explicitly.
@bmeck Can you point to something more specific?
And, no, I think the intent of async_hooks is to provide a more low-level approach to tracking async operations. That domains might be re-implementable on top of that would just be a nice effect.
@addaleax ... given the concerns on this.. how would you feel about marking this as experimental (assuming it lands) so that the specific implementation details can change without it being semver-major until we get more experience with the use?
The idea was that domains should be one day implementable ontop of async_hooks so that we can remove the domains code that is everywhere. that being said, it is not explicitly built as a direct replacement of any form (it contains zero error handling at all). (You can handle errors by attaching and unattaching uncaughtException handlers.)
Aside: off-topic?
@bmeck definitely not a gateway to reimplement domains - all involved are aware of https://gist.github.com/trevnorris/d19addf4aa09a0a31a4a14e90cb5c302 and no one likes domains here :)
For what it's worth the complains @misterdjules raises here are all legitimate. I recall spending countless hours on this and postmortem concerns in the past.
That said I also agree with @addaleax about promisify being the best course of action at the moment, it's a much simpler approach than #5020 attempted and I think it's our best shot and we owe it to the community.
I don't think postmortem concerns can truly be addressed (even now that we have stack traces) until async stack chains land (not just async stack traces). Note that the promise constructor issues swallowing errors people complained about in #5020 - do not exist in this PR :)
@jasnell I think that question might better go to @misterdjules as well. 😄 So far most of the concerns that have been voiced on the PR are about Promise support in core in general, and I don’t see that changing anytime soon.
That’s not to say I didn’t think of marking util.promisify as experimental as well … it’s just that so far I haven’t been able to see a notable advantage in that.
The only notable example I see with marking it experimental would be in giving us more flexibility in exactly how it's implemented after we get more implementation experience with it.
I think being experimental has been helpful for N-API, allowing us to make changes to the API more easily early on, with the plan to make it non-experimental once things settle down based on implementation experience.
@mhdawson Yea, I know what the experimental status is there for – and I’m not saying that we definitely won’t need to make adjustments to util.promisify(), it’s just that it looks to me like anything in that direction could be covered by adding options and sticking with normal semver rules. N-API has a lot more moving parts, I agree that it makes sense to use it there.
Are we willing to make the trade-off of safe error handling (and by association reliability and debuggability) versus popularity in Node core?
This PR means that we're unable to safely rely on the existing error handling semantics, namely not swallowing errors by default or affecting post-mortem debugging.
@yunong See also #12 (comment), but basically the big question for me remains: How does a feature like this change the way companies like Netflix use Node? It is opt-in, so I would assume you are concerned about increasing ecosystem popularity of Promise-based code. I don’t want to clutter this discussion by repeating myself, but isn’t that a trend we are seeing anyway, and one that is more likely to be influenced by the existence of e.g. native async/await than by this feature?
And re: swallowing errors by default, I know about the issues with that, but I’d just like to draw your attention towards nodejs/node#12010, a PR opened for throwing an error when an unhandled Promise rejection occurs (right now on garbage collection, but you’ll see in the thread that we’re still discussing the nuances there), as well as nodejs/node#12489, which would add support for Promises to domains (which obviously still not the same as callback-based error handling, but still).
This PR means that we're unable to safely rely on the existing error handling semantics, namely not swallowing errors by default or affecting post-mortem debugging.
Promises do not swallow errors and haven't since v1.3.4.
async function foo() {
throw new Error("Hi");
}
foo(); // Unhandled Rejecton: "Hi"This was fixed a long time ago in just because error swallowing is bad behavior. So I'm glad to tell you this particular concern has been addressed.
About postmortem - everything generally works - with nodejs/node#12489. The only issue is that exceptions might result in a core dump one frame later, at which point relevant buffers to debugging might have been touched. I don't think it's that bad since it's only a microtick so no other events have been handled by the time it happens. This again is a deliberate decision for this.
Promises are a fact at this point, more than half of our users use them if we believe surveys and the current state of affairs is very problematic. I, and tens of thousands of others have promises in production and have been able to debug our code (and take core dumps) just fine - what you're describing has been mostly fixed :)
As for your code, no one is suggesting that you use promises all of a sudden - the goal is to enable users to use a certain language feature (async/await) rather than force them to use language features more slowly from userland. This is about enabling our majority of users to use the core APIs with what the language gave us.
I realize we're not all fans of promises, and I think the concerns you raise are important - but as a first step landing just enough promise support for enabling our users but without forcing anyone to use promises is amazing.
This PR means that we're unable to safely rely on the existing error handling semantics, namely not swallowing errors by default or affecting post-mortem debugging.
Promises do not swallow errors and haven't since v1.3.4.
async function foo() {
throw new Error("Hi");
}
foo(); // Unhandled Rejecton: "Hi"
This was fixed a long time ago in just because error swallowing is bad behavior. So I'm glad to tell you this particular concern has been addressed.
I believe that what @yunong meant by "not swallowing errors" is a bit stricter than emitting a warning at some point in the future. In your example above, the error is still handled (effectively "caught") and emitted later in a different form.
I agree it's better than nothing, but I think it might not suit the use case of users who expect the process to exit fast (that is, before a microtask, a nextTick callback, or other async operations have time to run).
FWIW, I had documented in a gist what I think constitutes the fundamental requirements for promises to not break post-mortem debugging support.
About postmortem - everything generally works -
I disagree, although I'd like to be proven wrong!. Let's consider this example:
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# cat /var/tmp/throw-in-async.js
function PostmortemDebuggingState() {
this.state = [];
}
process.on('unhandledRejection', function onUnhandledRejection() {
process.abort();
});
var postmortemDebuggingState = new PostmortemDebuggingState();
async function foo() {
postmortemDebuggingState.state.push('foo');
throw new Error("Hi");
}
foo();
process.nextTick(function bar() {
postmortemDebuggingState.state.push('bar');
});
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# node --version
v7.2.1
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# node --harmony-async-await /var/tmp/throw-in-async.js
1: node::Abort() [/opt/local/bin/node]
2: node::Abort(v8::FunctionCallbackInfo<v8::Value> const&) [/opt/local/bin/node]
3: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/opt/local/bin/node]
4: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/opt/local/bin/node]
5: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/opt/local/bin/node]
6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [/opt/local/bin/node]
7: 2cd02a5063a7
8: 2cd02a645b4d
9: 2cd02a507e55
10: 2cd02a61ced7
11: 2cd02a61cb05
12: 2cd02a507e55
13: 2cd02a64563f
14: 2cd02a644f9d
15: 2cd02a644af2
16: 2cd02a644787
17: 2cd02a62e219
18: 2cd02a62e04d
19: 2cd02a57c62b
20: 2cd02a578b1a
21: 2cd02a54a7c3
22: 2cd02a52aa81
23: v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::Object>) [/opt/local/bin/node]
24: v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/opt/local/bin/node]
25: v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/opt/local/bin/node]
26: v8::Function::Call(v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/opt/local/bin/node]
27: node::LoadEnvironment(node::Environment*) [/opt/local/bin/node]
28: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/opt/local/bin/node]
29: node::Start(int, char**) [/opt/local/bin/node]
30: _start [/opt/local/bin/node]
Abort (core dumped)
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# echo "::load /root/mdb_v8_amd64.so; ::findjsobjects -p state | ::findjsobjects | ::jsprint" | mdb /var/tmp/core.node.71546
mdb_v8 version: 1.1.4 (release, from 3a6fad0)
V8 version: 5.4.500.44
Autoconfigured V8 support from target
C++ symbol demangling enabled
{
"state": [
"foo",
"bar",
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
],
}
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]#
What happens is that the data that was used to store the state meant to be inspected with post-mortem debugging tools was modified after the error was thrown. In this case, the program adds to the data meant to be inspected post-mortem, which could already make root causing the original problem more difficult, but in general that data could be modified in any way, e.g deleted.
Did I miss something?
with nodejs/node#12489.
My understanding is that nodejs/node#12489 is not relevant to post-mortem debugging. Domains are orthogonal with post-mortem debugging. Using one does not impact one's ability to use the other. For the sake of keeping this discussion focused on the original topic, I would suggest not discussing that PR further in this issue.
The only issue is that exceptions might result in a core dump one frame later,
What do you mean by "frame"? Do you mean a stack frame, a libuv event loop tick, a V8 microtasks tick, something else?
at which point relevant buffers to debugging might have been touched. I don't think it's that bad since it's only a microtick so no other events have been handled by the time it happens.
Wouldn't other (as in, more than 0) microtasks be able to run between the time the error is thrown and the 'unhandledRejection' event is emitted?
Let's consider the same example as above, but modified to run another async function after the error is thrown in the first async function called:
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# cat /var/tmp/throw-in-async.js
function PostmortemDebuggingState() {
this.state = [];
}
process.on('unhandledRejection', function onUnhandledRejection() {
process.abort();
});
var postmortemDebuggingState = new PostmortemDebuggingState();
async function foo() {
postmortemDebuggingState.state.push('foo');
throw new Error("Hi");
}
async function baz() {
postmortemDebuggingState.state.push('baz');
}
foo();
baz();
process.nextTick(function bar() {
postmortemDebuggingState.state.push('bar');
});
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# node --harmony-async-await /var/tmp/throw-in-async.js
1: node::Abort() [/opt/local/bin/node]
2: node::Abort(v8::FunctionCallbackInfo<v8::Value> const&) [/opt/local/bin/node]
3: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/opt/local/bin/node]
4: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/opt/local/bin/node]
5: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/opt/local/bin/node]
6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [/opt/local/bin/node]
7: dc2153063a7
8: dc215447ccd
9: dc215307e55
10: dc21541ced7
11: dc21541cb05
12: dc215307e55
13: dc2154477bf
14: dc21544711d
15: dc215446c72
16: dc215446907
17: dc21542e219
18: dc21542e04d
19: dc21537c62b
20: dc215378b1a
21: dc21534a7c3
22: dc21532aa81
23: v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, bool, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, v8::internal::Handle<v8::internal::Object>) [/opt/local/bin/node]
24: v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/opt/local/bin/node]
25: v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/opt/local/bin/node]
26: v8::Function::Call(v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/opt/local/bin/node]
27: node::LoadEnvironment(node::Environment*) [/opt/local/bin/node]
28: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/opt/local/bin/node]
29: node::Start(int, char**) [/opt/local/bin/node]
30: _start [/opt/local/bin/node]
Abort (core dumped)
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]# echo "::load /root/mdb_v8_amd64.so; ::findjsobjects -p state | ::findjsobjects | ::jsprint" | mdb /var/tmp/core.node.6732
mdb_v8 version: 1.1.4 (release, from 3a6fad0)
V8 version: 5.4.500.44
Autoconfigured V8 support from target
C++ symbol demangling enabled
{
"state": [
"foo",
"baz",
"bar",
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
hole,
],
}
[root@15f20df8-70ef-ec46-a085-b7205401ddc7 ~]#
It seems that the new async function baz ran, and was able to mutate the state that is meant to be inspected post-mortem, which seems to be a problem.
As for your code, no one is suggesting that you use promises all of a sudden - the goal is to enable users to use a certain language feature (async/await) rather than force them to use language features more slowly from userland. This is about enabling our majority of users to use the core APIs with what the language gave us.
I realize we're not all fans of promises, and I think the concerns you raise are important - but as a first step landing just enough promise support for enabling our users but without forcing anyone to use promises is amazing.
My concern is about the impact of node's built-in APIs supporting the promises' error handling model by providing a way for its users to use the built-in APIs as promises.
I believe that this would create a significantly higher risk for post-mortem debugging users to indirectly use third party modules that use promises, and thus use an error handling model that is not compatible with their use case.
As a result, I think that implementing what is described in this issue, without solving the fundamental difference between the promises and non-promises error handling models, will have a significantly negative impact on the post-mortem debugging use case.
I would like to clarify my position by stating that I would much rather not oppose the addition of promises-based APIs in node's built-in API, and I'd be more than happy to change my current position. But unless we can show that all significant post-mortem debugging use cases are addressed, I'll have to consider that we're willing to break node's post-mortem debugging support. I don't think that this would be a good message to send to our users in general.
What happens is that the data that was used to store the state meant to be inspected with post-mortem debugging tools was modified after the error was thrown.
The asynchronous delivery of the 'unhandledRejection' event - while good for preventing runaway recursion - is not helpful for postmortem tooling, I agree.
Perhaps a good way forward is to extend --abort_on_uncaught_exception to also cover unhandled rejections.
I don't want to reply for @yunong, but as a post-mortem debugging user myself.
How does a feature like this change the way companies like Netflix use Node? It is opt-in, so I would assume you are concerned about increasing ecosystem popularity of Promise-based code.
It is opt-in, but adding what this issue describe to node's built-in API would make the node platform officially support an error model that is incompatible with some of the best practices that have been developed, and that enable the post-mortem debugging use case.
So my concern is that it would make it more difficult than it already is for post-mortem debugging users to not run code that is incompatible with their use case.
I don’t want to clutter this discussion by repeating myself, but isn’t that a trend we are seeing anyway, and one that is more likely to be influenced by the existence of e.g. native async/await than by this feature?
I agree it is a trend that we're seeing, but at the same time the node runtime is a compelling platform for some users because it allows them to use an error model compatible with post-mortem debugging.
I find it disturbing that it seems the node project is considering to break the post-mortem debugging use case without putting a lot of effort on finding a solution to make promises work with that use case.
And re: swallowing errors by default, I know about the issues with that, but I’d just like to draw your attention towards nodejs/node#12010, a PR opened for throwing an error when an unhandled Promise rejection occurs (right now on garbage collection, but you’ll see in the thread that we’re still discussing the nuances there)
I would think that throwing an error on garbage collection of rejected promises has the same issues than the ones I presented above. Among other potential problems, it seems the process would exit at a time that is not necessarily the time at which the error was thrown, and as a result the generated core files might not be usable for post-mortem debugging.
I believe these potential problems and others have been presented by @chrisdickinson in that PR already.
I'd be happy to be convinced that this PR solves all the post-mortem debugging concerns and change my mind though.
as well as nodejs/node#12489, which would add support for Promises to domains (which obviously still not the same as callback-based error handling, but still).
Right, that PR is orthogonal to the post-mortem debugging concerns, I don't think it helps to discuss it further here.
Perhaps a good way forward is to extend --abort_on_uncaught_exception to also cover unhandled rejections.
I had started to explore solutions (with the help of others) at https://gist.github.com/misterdjules/2969aa1b5e6440a7e401#potential-solutions.
I had stopped that work when nodejs/node#5020 was closed without being merged, and IIRC the state of that work at the time was that I couldn't find a way to have a "--abort-on-unhandled-rejection" mode that would be compatible with the normal mode.
In other words, code that would not exit when throwing an error from within a promise would then make the process abort when running node with --abort-on-unhandled-rejection.
But this was before nodejs/node#8217 landed, and maybe now this design problem doesn't exist anymore if the node runtime moves towards exiting on unhandled promises rejection.
Shouldn't be too hard to make it work though, right? Check if --abort_on_uncaught_exception is specified in node.cc and in unhandledRejection() in lib/internal/process/promises.js call process.abort() if it is.
Shouldn't be too hard to make it work though, right?
It seems it would at least be possible, and probably not difficult, I agree.
Check if --abort_on_uncaught_exception is specified in node.cc and in unhandledRejection() in lib/internal/process/promises.js call process.abort() if it is.
It seems that method would rely on using SetPromiseRejectCallback. At the time I worked on this, I had found potential problems with using that method. I don't know if they still exist.
Sorry, I was perhaps a little too succinct there. What I mean is that:
-
You scan the command line for the
--abort_on_uncaught_exceptionflag. There is precedence for that, we already do it for a few other V8 flags. -
Expose that fact to JS land, e.g. by adding a fourth argument to the
process._setupPromises()callback. (A little neater than adding another property toprocessand leaves the door open for making it configurable at runtime.)
I would not recommend conflating "uncaught exceptions" from "unhandled rejections" - Promise.reject() is not an exception, but it is definitely an unhandled rejection. A separate flag would be much more explicit.
by adding a fourth argument to the process._setupPromises() callback
I want to make sure I understand what you mean. Do you mean making PromiseRejectCallback abort in case the flag passed to process._setupPromises() indicates that the runtime should abort on an unhandled promise rejection?
If that accurately describes what you're suggesting, I still think it would not fix the problems with the post-mortem debugging use case. The reason is that the PromiseRejectCallback function is called asynchronously, after all microtasks that had been scheduled have run when the stack frame where an error was thrown is not at the top of the stack. At least it is how it used to behave when I tried to use it for that purpose last time. Do you mean that that behavior changed, or am I missing something?
I believe that what @yunong meant by "not swallowing errors" is a bit stricter than emitting a warning at some point in the future. In your example above, the error is still handled (effectively "caught") and emitted later in a different form.
The above example dispatches an event that can be handled with a throw at unhandledRejection before any I/O has run. Note that there is a PR by @Fishrock123 to make it throw and abort on GC which would exit the process.
I agree it's better than nothing, but I think it might not suit the use case of users who expect the process to exit fast (that is, before a microtask, a nextTick callback, or other async operations have time to run).
I'm not sure I agree, the vast majority of promise users (who are a majority of Node users as a whole if we believe surveys) do not really use core dumps at all, and out of those who do - the number of cases a microtask passing has any meaningful consequence is much smaller. Again, this is entirely opt in and this PR just enables users to use a language feature directly.
I disagree, although I'd like to be proven wrong!. Let's consider this example:
I'd like to see a realistic use case where a microtask isn't artificially used to insert something before I/O callbacks. With a setImmediate or anything that would indicate any data processing that would not be the case. Of course I'd just pass a copy of the array with an error at which point we are able to avoid having to take a core dump altogether.
What do you mean by "frame"? Do you mean a stack frame, a libuv event loop tick, a V8 microtasks tick, something else?
I meant the microtask queue gets to run, not an event loop tick.
Let's consider the same example as above, but modified to run another async function after the error is thrown in the first async function called:
Right, as I said the microtask queue is depleted, I am not suggesting this does not cause errors to be taken as core dumps later - I'm saying that the benefit of said tooling is something the majority of our users care a lot less about than having a core API they can use with async/await.
It is opt-in, but adding what this issue describe to node's built-in API would make the node platform officially support an error model that is incompatible with some of the best practices that have been developed, and that enable the post-mortem debugging use case.
I disagree it's the "best practice", I think we have established in nodejs/post-mortem#18 that it's not really possible to take a core dump responsibly on every error that happens anyway.
Mainly because:
I believe that this would create a significantly higher risk for post-mortem debugging users to indirectly use third party modules that use promises, and thus use an error handling model that is not compatible with their use case.
The Node ecosystem is already full of modules that use promises. Bluebird has 14 million downloads and dependents like some of the most popular ORMS, build tools and server infrastructure tools. Similarly for Q (with 11 million). Note I'm ignoring native promises which are probably much more popular at this point.
This PR is just about making a utility that enables users to easily get a version of the API that's safe and fast for supporting async functions (which people use anyway). Making people get a bunch of third party tools to use our core APIs the same way they use every other API is making Node look bad and is costing a lot of programmers a lot of time.
If you want to blacklist them you can still delete Promise. This PR does not add promise support to the core APIs, it just enables users to do it easily and performantly.
The reason is that the PromiseRejectCallback function is called asynchronously, after all microtasks that had been scheduled have run when the stack frame where an error was thrown is not at the top of the stack.
I think you already figured this out but for posterity: the event is synchronous but promises.js defers it with process.nextTick().
@bnoordhuis yes and it must not cause an abnormal completion according to spec: https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker
The specific opt-in on this is what makes it the most attractive. I fully understand that there are folks who have deep technical objections to the design and use of Promises in the language, the way they have been implemented, and the limitations they have with regards to Post mortem debugging, but having a util.promisify() function added to core does not force anyone to make use of those things. 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. If this effort was forcing folks to make use of Promises, then I definitely would not be supportive, but it's not... nor is it trying to solve all of the problems that exist with Promises as currently defined.
A couple notes--happy to see this getting more complete with @addaleax 's PR:
- We've informally talked about the possibility of adding
promisifyat the TC39/ECMA-262/V8 level, but a common objection is thatpromisifyis too specific in its calling conventions. Many Web APIs take the callback as the first parameter or a middle parameter, rather than the last one aspromisifyexpects, and there's no obvious, clean way to generalize things. If it won't be in JavaScript by default, but if it's considered broadly important, then putting it in Node Core seems like another decent option. - @gsathya has had great results optimizing Promises, to the point that, if you take out the use of built-in
promisifyfrom the Bluebird benchmarks, native Promises perform at least as well as Bluebird promises. However, the benefit that built-inpromisifygave was huge. On top of not allocating closures, as @addaleax 's patch already does, avoiding C++/JS transitions has been important in making Promises fast. V8 extras may be a faster way to optimizepromisifythan using the V8 API since it avoids a JS/C++ transition; if extras are only available to Core, that could give a good reason to putpromisifyin Core.
@littledan a review of the PR would be much appreciated :) I'm not too concerned about performance not being better than bluebird initially since we can always ship fast promisify and make it faster in a follow up.
I've benchmarked the PR (stupidly, in my own app benchmark and not with the BB suite) and it significantly outperformes a naive new Promise which is good enough for me at this point. I intend to test util.promisify in our (BB) suite sometime in the coming weeks and validate this.
Thanks for all the performance work you (V8) have been doing on making promises fast enough for Node users by the way :) !
I would not recommend conflating "uncaught exceptions" from "unhandled rejections" - Promise.reject() is not an exception, but it is definitely an unhandled rejection. A separate flag would be much more explicit.
To clarify: in arguing for "crash on unhandled rejection", it is important to note that Promise.reject() would not immediately crash the program. The unhandled rejection handler is not called until microtasks are run, so there's an opportunity to correct handle rejections created in this fashion.
Or, rephrased: by itself, Promise.reject() is not an unhandled rejection until the next tick.
@chrisdickinson thanks for clarifying; that also means that throw foo in an async function also isn't an unhandled rejection until the next tick - which imo reinforces the argument that unhandled rejections are not uncaught exceptions, and thus should be under a separate option.
I've been reading this thread for a while now, and I have to say, I hear the term "post-mortem" a lot. I'm a node user, I'd like to think I'm an accomplished one at that. I made my first OS contribution to node a few days (yey me!) and I've had to look that term up before I understood what it's all about.
I've never relied on core dumps to debug, most probably, none of the people I know (save for @benjamingr, probably) even know how to do it, let alone do it effectively.
I do use Promises though, often. I understand them pretty well, I see how they're useful over the callback model, and I often find myself wishing that the core APIs were Promise based and not callback based. I find them easier to use, easier to debug, easier to isolate in tests. I feel they make my programs more descriptive and easier to follow, which I value more than debuggability and even performance in many cases.
So I'm under the impression that the vast vast majority of node users would take a Promisified API over post-mortem abilities, even in the case those were to be hindered. That's doubly true since the feature in question is basically an opt-in, rather than an opt-out.
So I'm under the impression that the vast vast majority of node users would take a Promisified API over post-mortem abilities, even in the case those were to be hindered.
While I agree a lot with what you are saying: I think the reason you’re not seeing much of people using Node’s post-mortem features is that it’s a different kind of user that is interested in them. A couple that have been brought up here are “Netflix, Pinterest, Uber, Twitter, Uber, Joyent, among others” – so, basically, large-ish companies that, when an unexpected error occurs, need to be able to know the exact state of the application in which that happened. Being able to get core dumps in that situation is a pretty important feature.
So, when you are saying “vast majority”, keep in mind that everybody has their own perspective and might not see that Node has a very, very broad set of users. :)
Oh, no disagreements there. I'm just saying that in my opinion, the needs of those companies/individuals shouldn't come before the needs of the rest of us "regular" folks. Especially when the feature is opt-in, and not forced upon anyone.
@misterdjules @yunong So, circling back to the issue with promises itself … basically, what you are mostly looking for is a way to get a coredump when a Promise handler throws or returns a rejected Promise, if and only if there is no explicit rejection handler present at that point. Is that correct?
So, for example:
Promise.reject(42).then(…); // does not crash, this is obviously intentional
Promise.resolve().then(() => {
throw new Error(); // hard-crashes, there is a handler but it doesn’t catch rejections
}).then(result => result+1);
Promise.resolve().then(() => {
throw new Error(); // does not throw, there is a catch handler
}).then(…).catch(e => …);I think that’s possible to do, but would require V8 changes (and maybe spec changes) because the default 2nd argument for .then() is actually a re-throwing error handler, which is currently counted as a rejection handler for the second example above.
I think that’s possible to do,
I don't think it is, we had a lot of discussion over it back at the #5020 PR days and without crippling promises this is impossible. The problem is not really the second argument to then, it's the fact that rejections are deferred to prevent timing bugs in code ("zalgo").
Having async stacks (like C# has and not just async stack traces, like we currently have) would go a long way.
One "solution" would be to add a flag that when enabled - node preemptively take a core dump whenever a rejection happens (handled or not) and then if it is unhandled crash the process and use the core dump we took earlier. We can, for example, do it only for the first rejection of a type in a cluster so performance doesn't get too bad.
I'd also like to point out we currently don't have a good solution for post-mortem debugging in production with callbacks anyway (and outside of production you can just reproduce it and look at the stack/heap with the debugger). 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. The "take a core dump first" solution is probably as viable as the solution suggestion in nodejs/post-mortem#18 by @Raynos.
Note that again, these post-mortem concerns are only pointing out something _problematic if the data is modified within a microtick and no I/O happens with the current hooks. Note that might change at some point.
Also note - that the ability to blacklist promises for post-mortem people remains exactly the same before and after this addition to the API.
Cross-posting my comment from nodejs/node#12442:
Are there any guidelines for what should go in the util module vs what should go elsewhere? it seems like there's a whole bunch of random stuff in util. I wonder whether it's worth making a separate cohesive module for async-related utilities (like promise-util or async-util or something).
@Daniel15 I think there is an extremely high barrier of entry to util now, it is very hard to add new features there and the need needs to be justified.
From what I've seen this is the criteria:
- Can it be implemented in userland - in this case,
util.promisifycan be implemented in userland but it's much slower to do so - when 3 promises are created for every request this starts to matter. - Does this enable users more than before? - I think that in this case, it allows users to use the async/await language feature with the core API without a third party dependency. Also without altering the core API.
- Is it future proof? - in this case yes, since implementing this will likely require privileged APIs for working. This is mentioned in the comments above - TC39 did the specification considering platforms will probably implement this sort of bridge.
I think introducing a new module to Node should have a much higher barrier. There aren't many things in util right now (all the util.isFoo stuff is deprecated and not really used).
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.
How is this an improvement over the "promisify" APIs already prominent in the ecosystem?
Responding to @benjamingr's points above:
- Performance: There's already a performance penalty to using promises on top of the callback API. I'm confident that ecosystem solutions can reach better performance than a core feature that only wraps the callback API. The only case I can think of where core offers a unique opportunity to move past these penalties is by offering a native promise API, which we could do eventually but this would sort of conflict with it so this particular direction may be counter-productive.
- "without a third party dependency": This is not a great argument. I don't think we know of any applications in production that don't include dependencies from npm. We usually see this as a feature, not a bug :)
In general, it is not a good practice to include things in core with the express purpose of reducing dependence on ecosystem solutions. Historically, the ecosystem has shown that it can evolve all-around better solutions to nearly every problem than Core. We tend to add things to core when they enable ecosystem innovation by standardizing lower level APIs. In this case, the lower level API is already defined by the Promise spec so I don't see how this particular case applies. - "Is it future proof?" -- I think it may not be, or at least it may in the future turn into a noop if Core exposes a native promise API.
The biggest concern I have here is the fact that we haven't seen the ecosystem coalesce around natively implemented promises (a lot of bluebird users are still out there). So we're in this spot where this feature is either forcing a standardization around the native promise or it's going to optionally allow any promise which we may actually not want to do in the future (there are some low level features we're adding to native promises that won't be possible in the ecosystem and we may want to push people into native promises in the future).
I think that this thread has been fairly de-railed by the debugging concerns around Promises, which I don't think are relevant except in the sense that we may have much better support for Promises in the future that is dependent on native Promises.
I empathize with the feeling that we should do something but often doing nothing is better for our uses than doing the wrong thing or even something half-assed. This feels like a half measure, and I think it sort of is by design, which would be fine if we were much more confident about what better Promise support in Node.js was going to look like. If I could see our destination then I could say whether or not this is a stepping stone to get there, but I don't know what that destination looks like yet and that makes it a lot harder to get behind this.
P.S.
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.
Side question: There was an effort by @chrisdickinson to spec out what exposing a Promise API for core would look like. Chris is no longer driving that but it made it pretty far along, with a lot of thought put into the best way to expose a parallel API. Does someone want to pick up where he left off?
The only case I can think of where core offers a unique opportunity to move past these penalties is by offering a native promise API, which we could do eventually but this would sort of conflict with it so this particular direction may be counter-productive.
That is actually one of the reasons why I like this approach – it allows core to provide custom implementations of the promise equivalents of callback methods instead of just wrapping them (see for example the timers changes in nodejs/node#12442, and that’s not even native code).
In general, it is not a good practice to include things in core with the express purpose of reducing dependence on ecosystem solutions.
I don’t see it as that, and I think many others don’t as well. I’d like to see us moving towards a world in which beginners learn to use Promise APIs before they learn the callback ones – that would reduce the entry barrier to learning Node in general a lot. (Actually, my engagement on this topic is substantially motivated by having that experience during NodeSchool and seeing over and over again how people try to wrap their head around callbacks – async/await is going to feel a lot more natural.)
So we're in this spot where this feature is either forcing a standardization around the native promise
Is that a bad thing? It might happen anyway because of async/await, and since Promise APIs are interoperable, I don’t feel uncomfortable doing this.
Does someone want to pick up where he left off?
Kinda? I would love to see that and I’d be willing to put a lot of work into it, but right now I wanted to take on a small-ish step forward and see if we can get Node core to commit to supporting Promises.
So we're in this spot where this feature is either forcing a standardization around the native promise
Is that a bad thing? It might happen anyway because of async/await, and since Promise APIs are interoperable, I don’t feel uncomfortable doing this.
I think I didn't write this clearly enough. What I mean is, we're forcing people to standardize on the "natively implemented promise" over promise libraries implemented in userland like bluebird.
Like I said, we may want to do that anyway eventually, but at the moment bluebird is substantially faster than the native implementation and possibly more widely adopted by Promise users in the ecosystem.
I think we're in a tough spot. On the one hand, we could accommodate these users by allowing them to set their preferred Promise implementation, but if we do that we lose one of the ways we might force standardization in the future if we have native-only features. On the other hand, if we push people into the native implementation now we may be pre-maturely forcing that standardization without a lot of objective reasons for preferring native promises over userland.
Kinda? I would love to see that and I’d be willing to put a lot of work into it, but right now I wanted to take on a small-ish step forward and see if we can get Node core to commit to supporting Promises.
I empathize, and agree, with the feeling that we just haven't done enough.
After watching some of the earlier Promise discussions it was clear that there were a lot of options for what great Promise support would look like in Node.js. My hesitation in taking a small step is that it's not clear yet what direction we want to go in as a final destination, and so it is hard for me to tell if this specific step moves us in the direction we want to end up in for the best Promise support.
That is actually one of the reasons why I like this approach – it allows core to provide custom implementations of the promise equivalents of callback methods instead of just wrapping them (see for example the timers changes in nodejs/node#12442, and that’s not even native code).
Interesting. I can see that I was wrong in assuming that this approach would limit our perf options. I still might be concerned that maintaining this alongside parallel API might be a maintenance burden but I think we can probably find a clever solution to that.
async/await already forces people to standardize on the natively implemented promise - whether node ships a util function or not, "use the native Promise most places" is the new reality, for bluebird users as well as everyone else.
@kyrylkov theses were the last benchmarks that I saw. https://blog.wikimedia.org/2017/02/17/node-6-wikimedia/
TBH, I think that this overhead is quite minimal in terms of IO delay, but is quite a lot for a low level primitive.
In some experiments I have done with v8 5.9 (I+TF) I saw no real difference in performance between callbacks and native promises. It was not really scientific and reproducible, so a more formal approach is needed.
@Mikael I'll later respond to the rest of the feedback, but as bluebird core I have to say:
- Native promises are as fast as bluebird in recent V8. They really did a lot of good work on them.
- Creation is slowing it down (why native loses to us in benchmarks). @littledan expanded on that both in the PR and here.
- This PR, on top of providing a uniform way to work with the language (bridge the language, core and the ecosystem) provides fast creation (no new Promise with closure). This is impossible without core or a native module.
In addition to being impossible in JS userland - promisify also enables our APIs to be uniformly consumed by async/await and lets other callback APIs do so cleanly (the custom parameter).
I also think I may have expressed myself poorly in terms of the dependency so apologies for that. It's about having a standardized way to consume callback APIs with a uniform way for them to declare how. I think this would help our users a lot.
Native promises are as fast as bluebird in recent V8. They really did a lot of good work on them.
🎆 Do you have a link that shows this in latest V8 somewhere? I'd like to get this out more, even get some tweets about it from @nodejs on twitter :)
After the responses as well as a few other individual conversations I want to say that all of my concerns have been addresses and this LGTM :)