Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add asyncify method to make async versions of synchronous methods #806

Closed
cappslock opened this issue Jun 24, 2015 · 6 comments
Closed

Add asyncify method to make async versions of synchronous methods #806

cappslock opened this issue Jun 24, 2015 · 6 comments

Comments

@cappslock
Copy link

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?

@cappslock
Copy link
Author

I'm happy to contribute a better version of this if it's desired.

@aearly
Copy link
Collaborator

aearly commented 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

@aearly aearly added the feature label Jun 24, 2015
@cappslock
Copy link
Author

Works for me. I do see some issues with that implementation though. From https://github.com/aearly/acomb/blob/master/index.js#L22:

acomb.asyncify = function asyncify(func) {
  return function (/*args..., callback*/) {
    var callback = _last(arguments),
      args = _initial(arguments);
    try {
      var result = func.apply(this, args);
      callback(null, result);
    } catch (e) {
      callback(e);
    }
  };
};

First, I think it's useful to assert that the callback function is defined in some way. Second, invoking that callback within the try block 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:

var result;
try {
    result = func.apply(this, args);
}
catch (e) {
    return callback(e);
}
return callback(null, result);

@aearly
Copy link
Collaborator

aearly commented 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 the replacer arg. 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
Copy link
Author

Yeah, that's a good point. I was not satisfied with the way I extracted the original arguments and callback in my implementation.

@aearly
Copy link
Collaborator

aearly commented Jun 28, 2015

Added in e794801.

@aearly aearly closed this as completed Jun 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants