Repository navigation
Add asyncify method to make async versions of synchronous methods #806
Description
Composing sync and async methods gets a little hairy. Consider a case where you're performing a waterfall of some methods, but one in the middle is synchronous. You have to do something like this:
async.waterfall(
[
asyncMethod1,
asyncMethod2,
]
function (error, data) {
if (error) {
// handle error
}
else {
var nextData = syncMethod3(data);
async.waterfall(
[
async.apply(asyncMethod4, data),
asyncMethod5
],
function (error, data) {
if (error) {
// handle error, probably in the same way as before
}
else {
doSomething(data);
}
}
);
}
}
);
I think this could be improved with the addition of a simple asyncify method. This method could take a synchronous function as an argument and return an asynchronous version. The returned function would invoke the original function or caught errors to the callback. Here is a naive implementation:
function asyncify (fn) {
return function () {
var callback = arguments[fn.length]
try {
var returnValue = fn.apply(null, Array.prototype.slice.call(arguments, 0, fn.length));
setImmediate(function () {
callback(null, returnValue);
});
}
catch (e) {
return callback(e);
}
}
}
With this, we can clean up the original code significantly:
async.waterfall(
[
asyncMethod1,
asyncMethod2,
async.asyncify(syncMethod3),
asyncMethod4,
asyncMethod5
],
function (error, data) {
if (error) {
// handle error
}
else {
doSomething(data);
}
}
)
Could something like this be added as an async method?
Activity
cappslock commentedon Jun 24, 2015
I'm happy to contribute a better version of this if it's desired.
aearly commentedon Jun 24, 2015
We're talking about this in #671 . Still deciding on what the exact name should be....
I'm probably going to pull in the implementation from https://www.npmjs.com/package/acomb#asyncify
cappslock commentedon Jun 24, 2015
Works for me. I do see some issues with that implementation though. From https://github.com/aearly/acomb/blob/master/index.js#L22:
First, I think it's useful to assert that the callback function is defined in some way. Second, invoking that callback within the
tryblock is asking for trouble. Any error thrown from that point forward would bubble up to your catch block, making debugging difficult. This is why in my example I used setImmediate, which I believe I've seen async use when invoking tasks before (and it has the added benefit of keeping things asynchronous). Alternately you could just do something like this:aearly commentedon Jun 24, 2015
You're right, try/catch around the callback is bad.
I still think slicing the arguments and always assuming the last arg is a callback is the way to go. It will get weird when you wrap a variadic synchronous function -- it will suddenly have a function that it didn't expect. For example, in the case of
async.asyncify(JSON.stringify)it would treat the callback as thereplacerarg. Also, you would primarily use this as an adapter for use in other async functions, where you can rely on a callback always being passed.cappslock commentedon Jun 25, 2015
Yeah, that's a good point. I was not satisfied with the way I extracted the original arguments and callback in my implementation.
aearly commentedon Jun 28, 2015
Added in e794801.