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

Adds race task #229

Merged
merged 4 commits into from
Mar 29, 2016
Merged

Adds race task #229

merged 4 commits into from
Mar 29, 2016

Conversation

flovilmart
Copy link
Contributor

Adds a race task, been using that code in a category in production apps for a while.
Heavily inspired by the taskForCompletionOfAll:

@nlutsenko
Copy link
Member

Wonderful! Yes, this is helpful for everyone, and is surfaced on Android as whenAny.
Meaning that here it would be taskForCompletionOfAnyTask: 😁

Will review code in detail in a little bit, but the first thing - can we rename it to taskForCompletionOfAnyTask:.
Slightly a longer name, I know, but overall - it describes intention better.

@flovilmart
Copy link
Contributor Author

taskForCompletionOfAnyTask: suggest that it requires completion involved. race: implies that the faster to return would be the one... I dunno, I like the conciseness of race (and I have it implemented in about 10 projects)

@flovilmart
Copy link
Contributor Author

@nlutsenko adding more tests, support for nullable array and rename the method

@nlutsenko
Copy link
Member

Going to nitpick on stuff here, but will try my best to review the entire thing right now.

@synchronized(lock) {
[exceptions addObject:task.exception];
}
}else if (task.error != nil) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Space before else.

@nlutsenko
Copy link
Member

Tried my best to review everything. There are few important pieces (usage of dispatch once predicate and constants instead of magic strings), but overall - it looks complete and nice.

Discussion piece:
Do you think it would be beneficial to have a taskForCompletionOfAnyTask:cancellationToken: in addition to this one? Most of our methods do have it, but I actually don't think it's that much helpful, plus it changes the semantics of how we treat cancellation, as we probably should cancel the whole task altogether if the token is cancelled.
Just throwing this here for discussion and curious to know your opinion on this.

@flovilmart
Copy link
Contributor Author

for the cancellation token, I believe that can be problematic. It's heavily inspired by + (instancetype)taskForCompletionOfAllTasks:(nullable NSArray<BFTask *> *)tasks; that don't have cancellation tokens, and I believe for the same reason. If tasks need a cancellation token, they should be cancellable by themselves don't you think?

@nlutsenko
Copy link
Member

Cancellable by themselves - maybe, though I sort of can see a situation where you would want to have an ability to cancel the entire chain that ends with AnyTask if say a person quite from the view, but still send the updates to the server. Up to you if you want to build another method for it here (we probably would want 2 methods one with token one without to match other APIs).

OSAtomicIncrement32Barrier(&cancelled);
}

if (task.completed && !task.faulted && !task.cancelled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like you can unify this as else statement on the above condition. Also, there is no way this task is not going to be completed here, since continueWithBlock: is called only after it's completed.

}];


BFTask * task = [BFTask taskForCompletionOfAnyTask:@[first, second]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove space before task.

@nlutsenko
Copy link
Member

Almost there. Few nits here and there and almost ready to go.
Also, you might want to use generics for those NSMutableArrays on the stack, since it will enforce that you can only add NSError/NSException to them.

@nlutsenko nlutsenko self-assigned this Mar 29, 2016
@nlutsenko nlutsenko merged commit 8623186 into BoltsFramework:master Mar 29, 2016
@nlutsenko nlutsenko added this to the 1.7.0 milestone Mar 29, 2016
@facebook-github-bot
Copy link
Contributor

@flovilmart updated the pull request.

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

Successfully merging this pull request may close these issues.

3 participants