util: add util.callbackify() #12712

refack · 2017-04-28T02:22:53Z

This is a first draft of an implementation of util.callbackify(function).
There is at least one issue that need ironing out: if and how to project the return value into multiple args...

Implementation inspired by @addaleax's #12442
Background discussion at: nodejs/CTC#109

Open discussion

Should we allow customization of the wrapping?

specifying a transform for the awaitable before the connection to the callback
projection of the returned value
tiying in the callback into the promise's life-cycle

Original commit descriptions

Add `util.callbackify(function)` for creating callback style functions

from functions returning a Promise/awaitable

Fixes: nodejs/CTC#109

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util

refack · 2017-04-28T02:28:00Z

For how to treat the returned value I choose to:

By default pass on as-is
Allow the user to ask us to splat the value
Allow the user to specify a list of keys that we will project out of the value
[Just thought of] allow the user to provide a projection lambda

P.S. treat this as pseudocode, since by the time my Windows machine finished compiling I'll be fast asleep, so I haven't verified it works :(

refack · 2017-04-28T02:30:10Z

/cc @chrisdickinson @benjamingr @Fishrock123 @nodejs/ctc

not-an-aardvark · 2017-04-28T03:06:50Z

For how to treat the returned value I choose to:

By default pass on as-is

Allow the user to ask us to splat the value

Allow the user to specify a list of keys that we will project out of the value

[Just thought of] allow the user to provide a projection lambda

I think option 1 is definitely best. The other options can easily be simulated in userland by using destructuring in the callback, so I don't think it's necessary to have special behavior to for spreading arrays/objects in core.

TimothyGu · 2017-04-28T03:08:09Z

lib/util.js

+      throw new TypeError('The [util.callbackify.custom] property must be ' +
+        'a function');
+    }
+    Object.defineProperty(fn, kCustomCallbackifyiedSymbol, {


I'd protect this with if (fn[kCustomCallbackifyiedSymbol] !== fn) as Object.defineProperty may be quite slow.

TimothyGu · 2017-04-28T03:10:33Z

lib/util.js

+      } else if (project) {
+        const allThere = project.all((k) => k in ret);
+        if (!allThere) {
+          throw new TypeError('Not all of the projected properties' +


I'd fill in undefined for properties that are not present.

I was wondering about that too.
Can we find some precedents? lodash maybe?

TimothyGu · 2017-04-28T03:11:01Z

lib/util.js

+
+callbackify.custom = kCustomCallbackifyiedSymbol;
+callbackify.toSplat = kCustomCallbackifyiedToSplat;
+callbackify.project = kCustomCallbackifyiedProject;


We should not make toSplat or project public for now, just like for promisify.

🤔 not sure, but keeping in line with promisify

TimothyGu · 2017-04-28T03:11:19Z

lib/util.js

@@ -1057,3 +1057,68 @@ exports._exceptionWithHostPort = function(err,
 // process.versions needs a custom function as some values are lazy-evaluated.
 process.versions[exports.inspect.custom] =
  (depth) => exports.format(JSON.parse(JSON.stringify(process.versions)));
+
+const kCustomCallbackifyiedSymbol = Symbol('util.callbackify.custom');


-kCustomCallbackifyiedSymbol +kCustomCallbackifiedSymbol

?

vkurchatkin · 2017-04-28T03:12:50Z

This seems unnecessarily complicated for such a simple feature. Is there an example where you would need all that customization?

not-an-aardvark · 2017-04-28T03:10:39Z

lib/util.js

+      } else if (project) {
+        const allThere = project.all((k) => k in ret);
+        if (!allThere) {
+          throw new TypeError('Not all of the projected properties' +


If we keep this property-picking option, then I think this error should be sent with cb(new TypeError(...)) rather than throwing and rejecting the Promise. If someone is using callbackify, then they're probably going to be listening for errors in the callback rather than listening for Promise rejections. (However, I don't think we should keep the property-picking option -- see #12712 (comment).)

Yep. Didn't think of that.

not-an-aardvark · 2017-04-28T03:15:35Z

lib/util.js

+      } else {
+        cb(null, ret);
+      }
+    }, cb);


This could cause confusing behavior if the original Promise rejects with a non-error:

function promiseFunction() { return Promise.reject(null); } util.callbackify(promiseFunction)((err, result) => { // err will be null, which could be confusing because the promise actually rejected. });

Bluebird seems to handle this by wrapping the rejection reason in an Error if it's falsy.

Maybe even if !err instanceof Error?

I don't think that would be a good idea because people sometimes intentionally reject Promises with objects. If we wrapped the objects in errors then we would lose information about the properties.

Adapted BlueBird snippet

vkurchatkin · 2017-04-28T03:17:18Z

lib/util.js

+    }
+    orig.call(this, ...args).then((ret) => {
+      if (toSplat) {
+        cb(null, ...ret);


What if cb throws? We probably want to avoid getting rejected promises from callbackified functions

What can we do? who can we tell?
Maybe process.emit('uncaughtException') or 'unhandledRejection'?

Maybe wrap the whole thing in process.nextTick?

So it'll go out of the Promise timeline?
I'd rather explicitly explode a la https://github.com/nodejs/node/pull/12712/files#diff-3deb3f32958bb937ae05c6f3e4abbdf5R1121

Not a good idea, it's better to allow the error to bubble. Otherwise you'll have to duplicate a lot of logic here for no good reason.

I'll check it out... (don't want to duplicate!)

not-an-aardvark · 2017-04-28T04:08:35Z

Is there a use case for having util.callbackify.custom? util.promisify.custom is useful because some functions have different callback conventions and so the default implementation wouldn't work, but that isn't the case with util.callbackify because all promise-returning functions just return a promise without separate conventions.

refack · 2017-04-28T04:16:31Z

Is there a use case for having util.callbackify.custom?

🤔 I thought of it as an 'escape hatch' to allow users to do whatever. Maybe adapt to old Promise implementations like our mpromise that doesn't have a catch

I have in mind the use case where this is used by dev A to adapt dev B's and dev C's APIs into a new uniform API, and there is a need to tweak each adaptation a little. So I'm not even sure about hiding callbackify.toSplat && callbackify.project. I think it makes the implementation more complete...

not-an-aardvark · 2017-04-28T04:28:20Z

I think having customization here is unnecessary given that it's trivial for users to customize the behavior in userland. Adding extra configurability where it's not needed seems like it will make the API bloated.

refack · 2017-04-28T13:22:09Z

P.S. do you think we should return the Promise
return orig.call(this, ...args).then((ret) => {
https://github.com/nodejs/node/pull/12712/files#diff-3deb3f32958bb937ae05c6f3e4abbdf5R1096
So the user could add a .catch or chain something?

benjamingr

Thanks for the work! awaiting changes.

benjamingr · 2017-04-28T13:42:33Z

lib/util.js

+
+const kCustomCallbackifiedSymbol = Symbol('util.callbackify.custom');
+const kCustomCallbackifiedToSplat = Symbol('util.callbackify.toSplat');
+const kCustomCallbackifiedProject = Symbol('util.callbackify.project');


I don't think we need these properties - they complicate the PR and with destructuring syntax and a uniform convention none of the benefits it would bring to util.promisify apply here IMO.

For example, it is required for fs.exists to work in promisify but there is no equivalent here.

Came to same conclusion. Gone in last commit.

benjamingr · 2017-04-28T13:43:35Z

lib/util.js

+
+  // we have two way to convert the returned value into multiple arguments
+  // either splat it or project some of it's keys
+  const toSplat = options.toSplat;


I don't think we should support this - with promisify we don't really have a choice for things like fs.exists but here destructuring syntax should be sufficient.

benjamingr · 2017-04-28T13:44:28Z

lib/util.js

+  const toSplat = options.toSplat;
+  const project = options.project;
+
+  const fn = function(...args) {


Using ...args args.pop and call with ...args is expensive here.

I would prefer to see arguments used, apply instead of call, and an inline slice instead of .pop for performance like bluebird does.

Since this will probably go on top of TF+I, is this still a major issue?

Just noting to keep it in mind, it's not yet clear when we'll be enabling TF+I. It's entirely possible that this could land before that but hard to say for sure.

I'll cook up some benchmarks (also look for DEOPTS)...

If you run this on top of TF+I (by checking out the appropriate branch) and the results show that these optimizations are meaningless - then we can drop them. I suspect they still affect execution speed on TF+I.

@benjamingr Fwiw, there have been PRs like #12183 opened by V8 people. I’m good with using rest parameters here, at least for this PR.

Interesting, @hashseed care to weigh in?

benjamingr · 2017-04-28T13:44:38Z

lib/util.js

+    }
+    var p = orig.call(this, ...args);
+    if (typeof options.transformer === 'function') {
+      p = options.transformer(p);


similar comment about not needing a transformer.

Rest parameters are at least as fast as arguments object, and avoids many performance cliffs. Spread is just as fast as Function.prototype.apply. If I remember correctly this applies to both Crankshaft and Turbofan.

@psmarshall and @bmeurer have more details though.

In any case, the runtime of this function is likely to be dominated by the chaining of the promise that follows. I wouldn't micro-optimize this just yet, and opt for the cleaner syntax.

benjamingr · 2017-04-28T13:46:20Z

lib/util.js

+      p = options.transformer(p);
+    }
+    p = p.then((ret) => {
+      if (toSplat) {


You have to call cb inside a process.nextTick in case it throws - you can't just abort the process or emit an event since it has to work with domains.

This is what bluebird and other libraries do with their implementations and it's the most reasonable thing to do.

I would also avoid all the if/else here and the projection - which users can just do in a second then.

benjamingr · 2017-04-28T13:47:22Z

lib/util.js

+        cb(null, ret);
+      }
+    }, (rej) => {
+      // !rej guard inspired by BlueBird https://goo.gl/t5IS6M


At bluebird, this improves debugging experience. In Node this is not something we can afford - converting non-errors into errors is useful for a library but it's not something the platform should do.

Ref: #12712 (comment)

I saw that, but we can't assume that if someone threw a falsey value it should be converted to an error. Bluebird does this to overcome the lack of stack trace but node should not make that decision for users. As a platform we should provide a more careful API.

To clarify, I didn't suggest doing this so that errors would have a stack trace -- I suggested it because null is a special error value in callbacks which means "no error occurred". If we don't error-wrap, there is no way for a callback consumer to distinguish between "the promise rejected with null" and "the promise fulfilled with undefined".

benjamingr · 2017-04-28T13:47:48Z

lib/util.js

+        rej = newRej;
+      }
+      cb(rej);
+    }).catch((e) => process.emit('uncaughtException', e));


Ditto, this catch is incorrect, and the error must be emitted in nextTick, otherwise this breaks post-mortem debugging.

I'm looking into this (and the nextTick)
Ref: #12712 (comment)
P.S. .catch happens on nextTick

@refack the problem isn't that catch happens on nextTick, it's that if you abort on unhandled exception the error would be thrown properly and the heap would not be possibly corrupted.

(Also, catch happens after nextTick, not during)

So you want an "explosion", callback throwing style? Makes sense.

refack · 2017-04-28T13:51:01Z

Last commit goes full-on with the options.
For your consideration...

refack · 2017-04-28T13:57:01Z

lib/util.js

+      }
+      cb(rej);
+    }).catch((e) => process.emit('uncaughtException', e));
+    return p;


Should we return the promise?

No, we should return undefined

It gives the user a false sense that the promise is actually somehow related to the callback's execution and creates a confusing API in which the user might expect the callback throwing to reject the promise.

This is why we switched from returning the promise to returning undefined in bluebird.

I'm going to document that

Basically, people were doing things like the following with BB:

await promise.asCallback((err, data) => { console.log("2"); }); console.log("1");

And expected 2, 1 to log (and got 1,2), not to mention people thinking that if they nest callbacks it will somehow magically wait for them.

Documented.
But added an option to opt-in to such behaviour 😉

not-an-aardvark · 2017-04-28T17:11:30Z

Last commit goes full-on with the options.
For your consideration...

I don't think this is necessary either, for the same reasons as #12712 (comment). I think we should just remove these options and the related functionality.

sam-github · 2017-04-28T17:15:01Z

No docs.... no comment (how could I if I don't know what its supposed to do?).

No node API returns promises, does it? What is the API for? (see above.... "no docs").

addaleax · 2017-04-28T17:16:49Z

lib/util.js

+    const cb = args.pop();
+    if (typeof cb !== 'function') {
+      throw new TypeError('The first argument to the callbackified function ' +
+        'must be a callback style function');


last argument?

addaleax · 2017-04-28T17:18:05Z

lib/util.js

+      p = options.transformer(p);
+    }
+    p = p.then((ret) => process.nextTick(onFulfilled, {ret, options, cb}),
+               (rej) => process.nextTick(onRejected, {rej, cb}));


Is there any reason for creating objects here? Just passing them as 2 or 3 extra arguments to nextTick should be fine

I had something else in mind, but now you're obviously right. Fixed.
(p.then((ret) =>{ret, options, cb}).then(onFulfilled))

refack · 2017-04-29T13:51:33Z

While the options is still in debate, I added option.catch allowing the user to tie-in the callback to the underlying promise's life cycles and, hance also returning the promise in this case.

refack · 2017-04-29T14:22:05Z

With latest comments incorporated IMHO two options have a meaningful use case

project - when the expected consumer is a user of a mixed API and expects uniform callback signatures (where the wrapped awaitable is a part of an API made by 3rd party)

catch - opt-in to incorporate the callback into the lifecycle of the underlying awaitable, i.e. having

await util.callbackify(RunAsync, {catch: ture})((err, data) => {
   console.log("1");
});
console.log("2");

print 1, 2

benjamingr · 2017-05-01T07:28:20Z

@refack catch should be on my default (otherwise what, no errors?) and project can just be something you call on the function later.

I think both of these still complicate the PR beyond scope, and may be added later if we really want them for some reason.

addaleax · 2017-06-17T10:41:34Z

@refack Can you backport (together with 873e2f2)?

Add `util.callbackify(function)` for creating callback style functions from functions returning a `Thenable` PR-URL: nodejs#12712 Fixes: nodejs/CTC#109 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

The `FALSY_VALUE_REJECTION` error code added by nodejs#12712 did not have the `ERR_` prefix, nor was it added to the errors.md documentation. Add the prefix in for consistency. PR-URL: nodejs#13604 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Add `util.callbackify(function)` for creating callback style functions from functions returning a `Thenable` Original-PR-URL: #12712 Fixes: nodejs/CTC#109 Original-Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Original-Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Original-Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com> Original-Reviewed-By: Anna Henningsen <anna@addaleax.net> PR-URL: #13750 Reviewed-By: Anna Henningsen <anna@addaleax.net>

The `FALSY_VALUE_REJECTION` error code added by #12712 did not have the `ERR_` prefix, nor was it added to the errors.md documentation. Add the prefix in for consistency. Original-PR-URL: #13604 Original-Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Original-Reviewed-By: Timothy Gu <timothygu99@gmail.com> Original-Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Original-Reviewed-By: Anna Henningsen <anna@addaleax.net> Original-Reviewed-By: Refael Ackermann <refack@gmail.com> Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Original-Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> PR-URL: #13750 Reviewed-By: Anna Henningsen <anna@addaleax.net>