Skip to content
This repository has been archived by the owner on Mar 31, 2018. It is now read-only.

util.promised - Callbacks and async/await #5

Closed
benjamingr opened this issue Feb 15, 2016 · 22 comments
Closed

util.promised - Callbacks and async/await #5

benjamingr opened this issue Feb 15, 2016 · 22 comments

Comments

@benjamingr
Copy link
Member

With async/await landing in v8 callback users should not have their workflows harmed.

I'm wondering if core should offer a util.promised method that wraps a callback returning method in such a way it can be used with async/await - effectively allowing non-promise users interact with async/await. It would work similarly to Promise.fromCallback:

async function doBar() {
    var result = await promised(fs.readFile)("hello.txt");
}

I'm not sure about the name since promised might come off as polarizing to callback users who would not want to be aware of the internals of async/await using promises.

Also, it's worth mentioning it doesn't have to return an actual promises but rather just a thenable since those are consumed by async/await.

@benjamingr benjamingr changed the title util.promise - Callbacks and async/await util.promised - Callbacks and async/await Feb 15, 2016
@chrisdickinson
Copy link
Contributor

We may also consider that this wrapper should cast returned promises from the wrapped API into native promises — to make it easier to consume promises from, e.g., knex, if one doesn't want knex's bluebird Promises to propagate through their code.

@chrisdickinson
Copy link
Contributor

Marking this as "not-blocking" for now. If anyone objects, please let me know and I'll be happy to change it!

@petkaantonov
Copy link
Contributor

What about asAwaitable? And talk about awaitables instead of promises.

@phpnode
Copy link

phpnode commented Feb 15, 2016

What about just awaitable. I'm not sure it should assimilate thenables - a thenable can already be awaited, so they should just pass through. Assimilation is taken care of by Promise.resolve().

const get = awaitable(function(url, cb) {
  request(url, cb);
});

const get2 = awaitable(get);

get === get2; // true because get is already thenable

async function go () {
 const data = await get("http://example.com/");
 return data;
}

@benjamingr
Copy link
Member Author

Yes, awaitable and awaiters instead of promises sounds like very good terminology to me. We don't need a full promise implementation or actual promises at all for this :)

@MadaraUchiha
Copy link

So... you're basically (almost) saying that you want bluebird's Promise.promisify(fn) in core.

@nolanlawson
Copy link

I think it's a bit silly to avoid the word "promise" since that's fundamentally what async/await is about – e.g. an async function is just a function that returns a promise. Also, the word "promise" will be inevitable when writing async/await code:

// `Promise.all` has replaced the now-defunct `await *`
var results = await Promise.all(arrayOfPromises);

@benjamingr
Copy link
Member Author

So... you're basically (almost) saying that you want bluebird's Promise.promisify(fn) in core.

Not at all. promisifys goal is to convert a callback API to a promises one. This is to get async/await working with callback APIs.

I think it's a bit silly to avoid the word "promise" since that's fundamentally what async/await is about

Technically we could have an Awaitable.all but I do agree that's kind of silly. We really want to accommodate callback users so they don't get "left behind" in the past. I think we need more feedback from these users.

@MadaraUchiha
Copy link

Yes, we do.

The way I see it, the vast majority of people who aren't using Promises would not use async functions either because they either

  1. Don't know enough to progress beyond the basics of what the tutorials taught them
  2. Are forced to support browsers without Promise support (and thus, without async/await support)

There probably are cases where people just choose (for some reason) to not use Promises in their code, be it by technical debt or a general dislike for Promises. If that's the only audience we're aiming at, I don't think it's worth the effort of putting it into core. Just use a library or implement it yourself.

@benjamingr
Copy link
Member Author

@MadaraUchiha

If that's the only audience we're aiming at, I don't think it's worth the effort of putting it into core. Just use a library or implement it yourself.

There are plenty of people who choose not to use promises. These people should be able to enjoy async/await and making sure they have a good programming experience when async/await lands is a priority.

@chrisdickinson
Copy link
Contributor

@benjamingr:

Not at all. promisifys goal is to convert a callback API to a promises one. This is to get async/await working with callback APIs.

Given that we'd have to introduce a third API (in addition to the currently-internal but externalizable promisify and callbackify) to turn callback-based functions into .then-ables, I believe it would be best to focus on how best to expose the internal promisify function to users. One of the stated goals of exposing a Promise-returning API from core is to address the needs of upcoming async/await users, and I don't see us preserving any value for ourselves (or our users) by returning non-Promise objects.

@benjamingr
Copy link
Member Author

@chrisdickinson I tend to agree with that too but I mostly use promises so I don't have that problem.

I'd like to know if callback proponents like @Raynos or @rvagg are interested in such an API and if they intend to use async/await at all.

@nolanlawson
Copy link

There are plenty of people who choose not to use promises. These people should be able to enjoy async/await

Choosing to use async/await but not Promises is like choosing to be a vegetarian but eating a big corn dog. 😉 Just because you can't "see" the meat (Promises) doesn't mean it's not there...

@omsmith
Copy link

omsmith commented Feb 16, 2016

Benefits of making util.promised public:

  • Can be used for 3rd party libraries
  • Can ignore the core-provided promise modules
    • Or could "replace" core-provided promise modules, technically, I guess

Cons of making util.promised public:

  • "Encourages" (at least in the example this issue proposed ) inefficient use of the function
    • (i.e. proper usage would be const awaitableReadFile = promised(fs.readFile); async function() { await awaitableReadFile(...); } in order to avoid recreating the shim every time)
      • Memoizing promised would make it less horrible

@Raynos
Copy link

Raynos commented Feb 16, 2016

@benjamingr as a callback user I want whatever is the most performant.

I suspect async/await will be too slow and I care more about performance then I care about synchronous looking code.

@benjamingr
Copy link
Member Author

@Raynos I presume that if it's implemented the same way as generators it would be fast. Slower than raw callbacks optimized, faster than things that use modules like async.

I doubt you'd have actual performance gains from it since you already probably avoid closures, pass only needed parameter and such - but I suspect most users who use closures in their code would see a small performance boost from async/await.

In either case, I doubt there would be a noticeable performance discrepancy between code written with await and code written without.

Are you saying that as a callback user this completely disinterests you? If it's within 20% speed of a "raw callback" solution would that change it?

We have everyone involved here, from the person implementing async/await in v8 to the people working on this PR to the people working in post-mortem and the error people.

I'm asking what you'd like to see "in your wildest dreams". Of course "I don't care about any of this or synchronous looking code" is also a completely valid response.

@Raynos
Copy link

Raynos commented Feb 16, 2016

@benjamingr

Please benchmark before making statements. I wouldn't be suprised if generators are an ORDER OF MAGNITUDE slower then callbacks.

If I can get a 2x perf boost from not using async/await then i'll use callbacks.

If async/await is fast then I'll use it. I think it's very nice, especially for authoring integration tests or implementing endpoints.

@phpnode
Copy link

phpnode commented Feb 17, 2016

@Raynos this goes both ways. There are lots of benchmarks out there that show that callbacks and generators are much closer than that, e.g. the bluebird benchmarks. If you have evidence to the contrary please provide it.

@benjamingr
Copy link
Member Author

@Raynos

Please benchmark before making statements. I wouldn't be suprised if generators are an ORDER OF MAGNITUDE slower then callbacks.

I wouldn't have been surprised either but if you consider their implementation as state machines they actually make for some pretty efficient code. Mainly because no closures are needed to write code that would have been convenient to write closures with. So I assume you would not gain a performance boost since you probably avoid nested callbacks anyway and name your callbacks - but everyone who nests callbacks will get a performance boost.

For example, I'd bet that:

for(var i = 0; i < arr.length; i++) {
    yield fn(arr, _); // _ substitutes CB and is a parameter is a generator
}

Is faster than a recursive alternative like:

function sequence(fn, arr, cb) {
    function run(results) {
       if(results.length === arr.length) cb(null, results);
       fn(arr[results.length], (err, data) => { 
            if(err) return cb(err);
            results.push(data);
            run(results);
       });
    }
    run([]);
}

You would not write code like sequence above, so again - I doubt you'd get a performance boost but I wouldn't rule them out altogether - especially in paths in the code that are not hot.

Now - if you were to use async/await what syntax would you like?

@petkaantonov
Copy link
Contributor

Naming callbacks doesnt get rid of any allocation, apis dont provide a way to pass a context object so closures are unavoidable

@benjamingr
Copy link
Member Author

@petkaantonov I mean something like this:

function a(cb) {
    getV((err, data) => err? cb(err) : b(data, cb));
}
function b(v, cb) {
   // the closure over 'v' inside getV does not need to exist here, unlike if we nest
}

They're not completely avoidable, but it minimizes it significantly for the common user.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 9, 2016

Related: nodejs/CTC#12

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants