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

`Promise.map` calls callback synchronously in bluebird v3.x #1148

Closed
overlookmotel opened this Issue Jun 27, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@overlookmotel
Contributor

overlookmotel commented Jun 27, 2016

  1. What version of bluebird is the issue happening on?

v3.0.0-3.4.1

  1. What platform and version? (For example Node.js 0.12 or Google Chrome 32)

Node v6.2.1, OS X 10.9.5

  1. Did this issue happen with earlier version of bluebird?

No


Promise.map() calling callback sync/async

The behavior of Promise.map() seems to have changed between bluebird v2.10.2 and v3.0.0.

In v2.x, the callback is always called asynchronously; in v3.0.0-3.4.1 the callback is called synchronously for literal values of the array.

var arr = [ 1, 2, 3 ];
Promise.map( arr, function(v) {
    console.log(v);
} );
console.log('next sync statement');

With bluebird v2.10.2:

next sync statement
1
2
3

With bluebird v3.x:

1
2
3
next sync statement

Resolved promises in the array are also mapped over synchronously. e.g. arr = [ 1, Promise.resolve(2), 3 ], gives the same result.

But any unresolved promises in the array are awaited before calling the callback on that item. This causes some puzzling behavior:

var arr = [ 1, Promise.resolve(2).tap( function() {} ), 3 ];
Promise.map( arr, function(v) {
    console.log(v);
} );
console.log('next sync statement');

Outputs:

1
3
next sync statement
2

And also:

var iterator = (function*() {
    yield 1;
    yield Promise.resolve(2).tap(function() {});
    yield 3;
})();

Promise.map( iterator, function(v) {
    console.log(v);
} );
console.log('next sync statement');
1
3
next sync statement
2

This behavior is the same with Promise.filter() too.

Questions

  1. Was this change in behavior intended? (If so, I feel it should be noted in the docs)
  2. If it wasn't intended, would this be considered Zalgo?
  3. Is it inconsistent that Promise.mapSeries() doesn't call the callback syncronously for the first round?
@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Jun 27, 2016

Contributor

Prototype method also calls the callback synchronously if called on a resolved promise in bluebird v3.x:

Promise.resolve( [1, 2, 3] ).map( function(v) {
    console.log(v);
} );
console.log('next sync statement');
var p = Promise.resolve( [1, 2, 3] ).tap( function() {} );

setImmediate( function() {
    p.map( function(v) {
        console.log(v);
    } );
    console.log('next sync statement');
} );

Both of the above produce output:

1
2
3
next sync statement
Contributor

overlookmotel commented Jun 27, 2016

Prototype method also calls the callback synchronously if called on a resolved promise in bluebird v3.x:

Promise.resolve( [1, 2, 3] ).map( function(v) {
    console.log(v);
} );
console.log('next sync statement');
var p = Promise.resolve( [1, 2, 3] ).tap( function() {} );

setImmediate( function() {
    p.map( function(v) {
        console.log(v);
    } );
    console.log('next sync statement');
} );

Both of the above produce output:

1
2
3
next sync statement
@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Jun 29, 2016

Contributor

@benjamingr I see you've labelled this issue as a bug. Does that mean that the synchronous calling of callbacks is not the intended behavior?

I'm working on a new shim of bluebird to support CLS https://github.com/overlookmotel/cls-bluebird/tree/refactor and it'd be useful to know how to handle this.

Contributor

overlookmotel commented Jun 29, 2016

@benjamingr I see you've labelled this issue as a bug. Does that mean that the synchronous calling of callbacks is not the intended behavior?

I'm working on a new shim of bluebird to support CLS https://github.com/overlookmotel/cls-bluebird/tree/refactor and it'd be useful to know how to handle this.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jul 2, 2016

Owner

Yes it is a bug instead of an intended change in 3.x

Owner

petkaantonov commented Jul 2, 2016

Yes it is a bug instead of an intended change in 3.x

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Jul 2, 2016

Contributor

@petkaantonov Thanks for your answer.

If it's going to be fixed, would this be considered a semver major change?

I mean while I do think it can be considered a bug and so only requiring a semver patch release, it's not specified in the docs whether the callback is meant to execute sync or async - so from that point of view it's ambiguous. But, more importantly, it's quite possible fixing this will break peoples' code.

Contributor

overlookmotel commented Jul 2, 2016

@petkaantonov Thanks for your answer.

If it's going to be fixed, would this be considered a semver major change?

I mean while I do think it can be considered a bug and so only requiring a semver patch release, it's not specified in the docs whether the callback is meant to execute sync or async - so from that point of view it's ambiguous. But, more importantly, it's quite possible fixing this will break peoples' code.

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

I've changed my mind on whether this should be a semver major change. I now think this should be considered a bug and could be fixed in v3.x.

It seems to me that the fundamental rule of Promises A+ spec is that callbacks chained onto a promise are called async (avoiding Zalgo).

Although bluebird docs do not specify whether .map()'s callback should be called sync or async, bluebird conforms to the A+ spec, so it's reasonable to assume that additional methods such as .map() will follow the same behavior as A+ specified methods such as .then().

I think the following two should be considered as equivalent:

Promise.resolve( 1 ).then( function( value ) { console.log( value ); } );
Promise.resolve( [ 1 ] ).map( function( value ) { console.log( value ); } );

Therefore, my conclusion is that the fact they behave differently should be considered a violation of the spirit of the Promises A+ spec, and so could be fixed in a v3.x patch release.

What do you think @petkaantonov @benjamingr?

Contributor

overlookmotel commented Aug 31, 2016

I've changed my mind on whether this should be a semver major change. I now think this should be considered a bug and could be fixed in v3.x.

It seems to me that the fundamental rule of Promises A+ spec is that callbacks chained onto a promise are called async (avoiding Zalgo).

Although bluebird docs do not specify whether .map()'s callback should be called sync or async, bluebird conforms to the A+ spec, so it's reasonable to assume that additional methods such as .map() will follow the same behavior as A+ specified methods such as .then().

I think the following two should be considered as equivalent:

Promise.resolve( 1 ).then( function( value ) { console.log( value ); } );
Promise.resolve( [ 1 ] ).map( function( value ) { console.log( value ); } );

Therefore, my conclusion is that the fact they behave differently should be considered a violation of the spirit of the Promises A+ spec, and so could be fixed in a v3.x patch release.

What do you think @petkaantonov @benjamingr?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

I am leaning towards patch because it's a pretty "obvious" bug and not many people can be relying on the order being essentially random.

Owner

petkaantonov commented Aug 31, 2016

I am leaning towards patch because it's a pretty "obvious" bug and not many people can be relying on the order being essentially random.

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

OK brilliant. If you can point me to the vaguely right area of the code, I can try to make a fix.

I'd like to contribute fixes rather than just raising issues all the time, but I'm just rather terrified by the complexity of the codebase and fear changing something which would unintentionally break something else.

Contributor

overlookmotel commented Aug 31, 2016

OK brilliant. If you can point me to the vaguely right area of the code, I can try to make a fix.

I'd like to contribute fixes rather than just raising issues all the time, but I'm just rather terrified by the complexity of the codebase and fear changing something which would unintentionally break something else.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

It should be enough to create in map.js:

MappingPromiseArray.prototype._asyncInit = function() {
    this._init$(undefined, RESOLVE_ARRAY);
};

And then replace the this._init$ call in the last line of the constructor with:

async.invoke(this._asyncInit, this, undefined);

To get reference to async do var async = Promise._async at the top.

Owner

petkaantonov commented Aug 31, 2016

It should be enough to create in map.js:

MappingPromiseArray.prototype._asyncInit = function() {
    this._init$(undefined, RESOLVE_ARRAY);
};

And then replace the this._init$ call in the last line of the constructor with:

async.invoke(this._asyncInit, this, undefined);

To get reference to async do var async = Promise._async at the top.

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

I think the place where this could break peoples' code is where they're iterating over an array that contains only literal values (i.e. not promises) and rely on the callback being run sync:

var total = 0;
Promise.map( [1, 2, 3], function(num) {
    // do something sync
    total += num;

    // now do something async
    return fs.readFileAsync( 'file' + num + '.txt' );
} );
console.log( 'total:', total );

So at present this outputs total: 6. But after this change, it'll be 0.

The above example doesn't rely on randomness, but predictable sync.

Contributor

overlookmotel commented Aug 31, 2016

I think the place where this could break peoples' code is where they're iterating over an array that contains only literal values (i.e. not promises) and rely on the callback being run sync:

var total = 0;
Promise.map( [1, 2, 3], function(num) {
    // do something sync
    total += num;

    // now do something async
    return fs.readFileAsync( 'file' + num + '.txt' );
} );
console.log( 'total:', total );

So at present this outputs total: 6. But after this change, it'll be 0.

The above example doesn't rely on randomness, but predictable sync.

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

Thanks for the suggestions on how to fix. I'll have a go in next few days.

Contributor

overlookmotel commented Aug 31, 2016

Thanks for the suggestions on how to fix. I'll have a go in next few days.

petkaantonov added a commit that referenced this issue Sep 1, 2016

Merge pull request #1221 from overlookmotel/map-async
`.map` callback called async (closes #1148)
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 1, 2016

Collaborator

Thanks!

Collaborator

benjamingr commented Sep 1, 2016

Thanks!

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Sep 1, 2016

Contributor

No, thank you! I'm pleased to be able to actually contribute fixes rather than just raising endless issues.

Contributor

overlookmotel commented Sep 1, 2016

No, thank you! I'm pleased to be able to actually contribute fixes rather than just raising endless issues.

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