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

Bluebird.coroutine() is sometimes synchronous #1170

Closed
STRML opened this Issue Jul 26, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@STRML

STRML commented Jul 26, 2016

This is with latest Bluebird (3.4.1) and Babel (6.11.4).

Many Babel users are using Bluebird.coroutine() in combination with transform-async-to-module-method to have Bluebird shim async/await.

Unfortunately, it's not a perfect shim. The following code:

async function foo() {
  await syncFn();
  console.log(3);
}
async function syncFn() {
  return;
}
console.log(1);
foo();
console.log(2);

prints 1 3 2 with Bluebird.coroutine() and 1 2 3 with the default transform-async-to-generator method.

I've created this Phabricator ticket to track but it appears the issue lies here.

The above code transpiles to this, which I have cleaned up for readability:

var Bluebird = require('bluebird');

function foo() {
  return Bluebird.coroutine(function* () {
    yield syncFn();
    console.log(3);
  })();
}

function syncFn() {
  return Bluebird.coroutine(function* () {
    return;
  })();
}

console.log(1);
foo();
console.log(2);

This is easily runnable in any Node runtime that supports generators:

$ node syncCoroutines.js
1
3
2

Is this intended behavior?

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jul 26, 2016

Collaborator

Looks like a bug.

Collaborator

spion commented Jul 26, 2016

Looks like a bug.

@spion spion added the bug label Jul 26, 2016

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Jul 26, 2016

Collaborator

Minimal test case:

var Bluebird = require('bluebird');
var foo = Bluebird.coroutine(function* () {
    yield Bluebird.resolve();
    console.log(3);
})
console.log(1);
foo();
console.log(2);

Basically, "yield" will synchronously inspect the promise and continue execution immediately if its already resolved. Since no handlers are added to the promise in this case, the async trampoline is completely avoided.

Collaborator

spion commented Jul 26, 2016

Minimal test case:

var Bluebird = require('bluebird');
var foo = Bluebird.coroutine(function* () {
    yield Bluebird.resolve();
    console.log(3);
})
console.log(1);
foo();
console.log(2);

Basically, "yield" will synchronously inspect the promise and continue execution immediately if its already resolved. Since no handlers are added to the promise in this case, the async trampoline is completely avoided.

not-an-aardvark added a commit to not-an-aardvark/bluebird that referenced this issue Jul 28, 2016

not-an-aardvark added a commit to not-an-aardvark/bluebird that referenced this issue Jul 30, 2016

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 1, 2016

Contributor

Here's a very hacky workaround for until this gets fixed:

var shimmer = require('shimmer');
var AltPromise = Promise.getNewLibraryCopy();

shimmer.wrap(Promise, 'coroutine', function(original) {
    return function(generatorFunction, options) {
        if (typeof generatorFunction === 'function') {
            var generatorFunctionOriginal = generatorFunction;
            generatorFunction = function() {
                var generator = generatorFunctionOriginal.apply(this, arguments);
                var next = generator.next;
                generator.next = function() {
                    var ret = next.apply(this, arguments);
                    if (ret.value instanceof Promise && !ret.value.isPending()) ret.value = AltPromise.resolve(ret.value);
                    return ret;
                };
                return generator;
            };
        }

        return original.call(this, generatorFunction, options);
    };
});

(the problem only occurs when the yielded promise is an instance of the main bluebird Promise constructor)

Contributor

overlookmotel commented Aug 1, 2016

Here's a very hacky workaround for until this gets fixed:

var shimmer = require('shimmer');
var AltPromise = Promise.getNewLibraryCopy();

shimmer.wrap(Promise, 'coroutine', function(original) {
    return function(generatorFunction, options) {
        if (typeof generatorFunction === 'function') {
            var generatorFunctionOriginal = generatorFunction;
            generatorFunction = function() {
                var generator = generatorFunctionOriginal.apply(this, arguments);
                var next = generator.next;
                generator.next = function() {
                    var ret = next.apply(this, arguments);
                    if (ret.value instanceof Promise && !ret.value.isPending()) ret.value = AltPromise.resolve(ret.value);
                    return ret;
                };
                return generator;
            };
        }

        return original.call(this, generatorFunction, options);
    };
});

(the problem only occurs when the yielded promise is an instance of the main bluebird Promise constructor)

@spion spion closed this in #1175 Aug 3, 2016

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 4, 2016

Contributor

Amazing you guys fixed this so fast. Any idea when it's likely to get pushed to npm?

Contributor

overlookmotel commented Aug 4, 2016

Amazing you guys fixed this so fast. Any idea when it's likely to get pushed to npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment