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

replaced bluebird.all with bluebird.reduce to call allow in sequence #112

Merged
merged 1 commit into from
Aug 5, 2015

Conversation

yonjah
Copy link
Contributor

@yonjah yonjah commented Apr 8, 2015

_allowEx was using bluebird.all to call the allow method, but this will call methods in parallel
and can cause race conditions which might result in overriding some of the permissions.

bluebird.reduce will wait for a Promise to resolve before moving to the next element in the array

@willsr
Copy link

willsr commented May 25, 2015

+1

@manast
Copy link
Member

manast commented Aug 5, 2015

thanks for this nice addition

manast added a commit that referenced this pull request Aug 5, 2015
replaced bluebird.all with bluebird.reduce to call allow in sequence
@manast manast merged commit 79c6279 into OptimalBits:master Aug 5, 2015
@jurko-gospodnetic
Copy link
Contributor

Can anyone give an example of such a race condition?

We use node_acl with a redis back-end and do quite a lot of parallel ACL operations using when.all(). It gives us quite a measurable performance difference (an operation in a scenario we encountered a few days ago went down from taking 19 minutes to taking cca. 20 seconds just by making all the ACL operations run in parallel instead of in sequence).

From checking the code, all the back-end operations are guarded by transactions and should be safe from JavaScript style parallelization, i.e. when called from worker functions that do not get interrupted at random points but only register their successors to be called in some future JavaScript VM tick.

Or was this related to a problem with some specific back-end implementation?

@manast
Copy link
Member

manast commented Aug 8, 2015

Maybe I was a bit quick in accepting this patch, since as you say the backend should take care of it without any issues. Specially in the read case there are no known race conditions since all operations are atomic. When I accepted this patch I was thinking actually that a high number of parallel calls could imply too many parallel network calls which could in fact affect negatively the performance, but this is also something that should be taken care of the back end. So if the submitter does not have a good motivation for this patch I will revert it.

@yonjah
Copy link
Contributor Author

yonjah commented Aug 17, 2015

I tried looking at the code again and it might be an issue I can solve on my orm side of things.
unfortunately I didn't kept the tests I had failing and I doubt if I'll have the time to look at this issue seriously any time soon, but this probably can be reverted back to use Promise.all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants