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 Task Cancellation #89

Merged
merged 1 commit into from May 20, 2015
Merged

Add Task Cancellation #89

merged 1 commit into from May 20, 2015

Conversation

josephearl
Copy link

Initial CancellationToken & CancellationTokenSource implementation to match the C# API (system.threading.cancellationtoken and system.threading.cancellationtokensource) and the cancellation API on Android (BoltsFramework/Bolts-Android#53 and BoltsFramework/Bolts-Android#56).

@ghost
Copy link

ghost commented Apr 13, 2015

@josephearl This looks amazing!
Sorry for a swarm of comments, but all of these are nits.
Will happily merge this in once all of the nits are resolved.

Keep it up and i am looking really forward to see more of these awesome things!

@dimazen
Copy link

dimazen commented Apr 15, 2015

Is there any movements on this PR? I'd love to have cancellation support 🌞

@josephearl
Copy link
Author

I'll be updating it shortly

@dimazen
Copy link

dimazen commented Apr 15, 2015

@josephearl thanks!

@josephearl
Copy link
Author

Updated. Also added the ability to register a delegate to be notified when a token is cancelled.

@josephearl
Copy link
Author

@nlutsenko-fb can you re-run the build please?

No output has been received in the last ... minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

@ghost
Copy link

ghost commented Apr 15, 2015

Doing this right now... Thanks!

@ghost
Copy link

ghost commented Apr 15, 2015

@josephearl this looks even better!
In addition to nits (I am sorry about them) - I would recommend running a auto-indentation on every new file.

Also - it looks like there is a test failing :(

@josephearl
Copy link
Author

  • Added copyright
  • Formatted new code
  • Changed registerDelegate to registerCancellationObserverWithBlock
  • Typedef'd BFCancellationBlock

#import <Foundation/Foundation.h>

/*!
Represents the registration of a delegate with a cancellation token.
Copy link
Author

Choose a reason for hiding this comment

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

registration of a delegate

registration of a cancellation observer

@nlutsenko
Copy link
Member

Hey @josephearl looks it won't merge automatically (needs rebasing).
Would be awesome to finally have this feature 😋

@josephearl
Copy link
Author

Will do

@josephearl
Copy link
Author

Rebased and moved logic from CancellationTokenSource to CancellationToken, to match TaskCompletionSource and Task.

@nlutsenko nlutsenko self-assigned this May 14, 2015
@nlutsenko
Copy link
Member

@josephearl This looks great! Few comments, but I am very tempted to merge it in...

@nlutsenko nlutsenko removed their assignment May 14, 2015
@josephearl
Copy link
Author

Will fix up this evening

@josephearl
Copy link
Author

Updated the docs and removed the token getter

@nlutsenko
Copy link
Member

This looks great! Merging in!

nlutsenko added a commit that referenced this pull request May 20, 2015
@nlutsenko nlutsenko merged commit 721a881 into BoltsFramework:master May 20, 2015
@emptyway
Copy link

Great job! thanks!

I suggest the "Task Cancellation" described in the Readme.md file should be updated with this implementation.

@caoer
Copy link

caoer commented Jun 23, 2015

great addition! :)

@nlutsenko @josephearl can I suggestion to make the cancellation token as the third parameter instead of 2nd? It can make the chaining method looks better. not sure how Android or C# doing with it.

change FROM

- (instancetype)continueWithExecutor:(BFExecutor *)executor
                               block:(BFContinuationBlock)block
                   cancellationToken:(BFCancellationToken *)cancellationToken;

TO

- (instancetype)continueWithExecutor:(BFExecutor *)executor
                   cancellationToken:(BFCancellationToken *)cancellationToken;
                               block:(BFContinuationBlock)block

@nlutsenko
Copy link
Member

We can do an extra one, since these are already here and we probably don't want to break existing users :(
But a great suggestion, love the trailing block (also it maps waaay better with trailing closures in swift)

@josephearl
Copy link
Author

Yes was just about to say we probably don't want to break the API. Sounds like a good addition.

@facebook-github-bot
Copy link
Contributor

@josephearl 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.

None yet

6 participants