'Cancel' for PromiseKit #899
Conversation
Sources/CancelContext.swift
Outdated
| } | ||
|
|
||
| class CancelItem: Hashable { | ||
| lazy var hashValue: Int = { |
There was a problem hiding this comment.
Wow, great catch! The CancelContext instance's hashValue could indeed be accessed by multiple threads at the same time.
I fixed CancelContext by making a Hashable wrapper class, where the wrapper instance will only be accessed by a single thread. Other threads make their own wrapper instances so this solves the thread safety issue with CancelContext.
CancelItem also has a lazy hashValue, but all dictionary operations involving CancelItem are protected by the barrier lock.
Admittedly this is a slightly convoluted. But I want to use ObjectIdentifier.hashValue for the CancelContext and CancelItem hash values, which then forces you to use lazy.
I'm very open to ideas for simplifying this, lmk if you think of anything!!
|
Should Guarantees be cancellable? My immediate reaction is: no. It’s not much of a guarantee if it may be canceled. |
|
Agreed, I like your idea to wrap the Guarantee with |
|
I've deleted CancellableGuarantee and put in a 'cancellable' global function that takes a Promise/Guarantee. This enabled me to delete a bunch of 'CC' functions. In the extensions I renamed methods with the 'CC' suffix to use a 'cancellable' prefix instead. A few questions --
|
|
When finished try to squash all of the commits .. git squash |
Codecov Report
@@ Coverage Diff @@
## v7 #899 +/- ##
=======================================
+ Coverage 87.3% 92.5% +5.2%
=======================================
Files 15 32 +17
Lines 793 1760 +967
=======================================
+ Hits 692 1627 +935
- Misses 101 133 +32
Continue to review full report at Codecov.
|
Well we can do that from GitHub nowadays. So don't worry about it. |
|
Ok cool -- I've been squashing, but after merging from the master repo and then some further changes on my part the commits got interleaved between mine and others. |
|
Ok! This is ready for another review. The file diffs were temporarily messed up because I pulled in the latest changes from master using git merge instead of git rebase (oops). I was able to fix it after reading up on git rebase -- quite interesting! The following are some questions, concerns, and feedback that has been incorporated. Holding a reference to a CancellableTask inside Promise/GuaranteeOf particular interest to review are the changes I made to Promise.swift and Guarantee.swift. I am stashing away the CancellableTask (if there is one) in the Promise/Guarantee, so that the 'cancellable' function can later figure out what needs to be cancelled. https://github.com/mxcl/PromiseKit/pull/899/files#diff-b9553d21255e0e41a9f3dfe3fd942d94 The CancellableTask is always stored regardless of whether 'cancellable' is later used. The concern is if holding a reference to the CancellableTask inside the Promise/Guarantee will cause any problems. I'd rather not hold on to this reference, but I couldn't think of another way to accomplish this. My reasoning is that the 'cancellable' function should always provide the ability to properly cancel the Promise or Guarantee passed to it. For example, cancelling a Promise from URLSession should always cancel the URLSessionTask in addition to rejecting the Promise with a CancellableError. You cannot prevent passing a given Promise or Guarantee to 'cancellable', therefore it must always work properly. Cancellable 'wrappers' for all Promises that have an underlying CancellableTaskI went ahead and created 'wrapper' methods for all Promises that have an underlying CancellableTask. For example, for the URLSession extension the following are equivalent: Using the 'wrapper' method: Using the The question is, should we provide these 'wrapper' methods? They are only useful for indicating which 'CC' methods and functions are goneI have remove all 'CC' methods and functions, and rely on the global
|
|
Just noticed that |
mxcl
force-pushed
the
master
branch
5 times, most recently
from
0e105ba to
4ff9962
Compare
|
K, this is way overdue, I have created a |
|
I've switched to the v7 branch. Let's merge in Garth Snyder's dispatch queue stuff first, then I'll merge his changes into my pull request (rebase!). I'll need to change the cancellable method signatures to match his new way of doing dispatch queues. I will deal with all this so the merge for you guys should be easy peasy. After merging the cancel stuff into the v7 branch, I will change all the cancellable extension Cartfiles to point at the v7 branch for CI testing. And when v7 gets merged into master, I'll change them again to point at PromiseKit 7. We can merge each extension when it is passing CI against PMK 7. All assuming cancellables get the nod for inclusion in PromiseKit 7 -- please let me know if you have any further feedback! |
|
The docs are pretty much complete and are ready for review! Looks like the Travis and Codecov are not running for pull requests on the v7 branch. Can CI be turned on for v7? Both Travis and Codecov should pass now. Travis is green for all the cancellable extensions! |
My remaining questions and concerns are:Holding a reference to a CancellableTask inside Promise/GuaranteeOf particular interest to review are the changes I made to Promise.swift and Guarantee.swift. I am stashing away the CancellableTask (if there is one) in the Promise/Guarantee, so that the 'cancellable' function can later figure out what needs to be cancelled. https://github.com/mxcl/PromiseKit/pull/899/files#diff-b9553d21255e0e41a9f3dfe3fd942d94 The CancellableTask is always stored regardless of whether 'cancellable' is later used. The concern is that holding a reference to the CancellableTask inside the Promise/Guarantee will cause some kind of problem. I'd rather not hold on to this reference, but I couldn't think of another way to accomplish this. My reasoning is that the 'cancellable' function should always provide the ability to properly cancel the Promise or Guarantee passed to it. For example, cancelling a Promise from URLSession should always cancel the URLSessionTask in addition to rejecting the Promise with a CancellableError. You cannot prevent passing a given Promise or Guarantee to 'cancellable', therefore it must always work properly. Cancellable 'wrappers' for all Promises that have an underlying CancellableTaskI went ahead and created 'wrapper' methods for all Promises that have an underlying CancellableTask. For example, for the URLSession extension the following are equivalent: Using the 'wrapper' method: Using the The question is, should we provide these 'wrapper' methods? They are only useful for indicating which How many Ls should be in cancelable/cancellable and canceled/cancelled?I have it as two Ls right now, matching the existing PromiseKit code. If we decide on one L then a few existing properties with two Ls and the CancellableError protocol would need to be marked as deprecated. |
|
I've removed the cancellable wrappers. After thinking about it, it is confusing to have two ways to do the same thing. Plus it adds maintenance work for the extensions. My remaining questions are:
Sorry, the git log for the pull request is a bit of a mess. Will try to fix this up when integrating the dispatch queue changes. The file diffs are good so it is all ready for review. |
|
Is this PR still active? If so, is it almost ready? |
|
It’s waiting on me. And will be a new major version of PromiseKit |
|
I think we can approach this by merging here and perhaps in PromiseKit/Foundation (v4) initially. Thoughts? |
|
Agreed that the level of duplication and intrusiveness is unfortunate. I'm happy to pare down in any areas that make sense and also a 'grand refactor' could help a lot!! I'm glad this can be of use and no worries about timing! I'll attempt to address all your points and am planning to add some tests to improve code coverage. |
|
Feedback is incorporated and code coverage is improved. One outstanding item: what name should be used for the 'cancellable' function? I'm done pending further feedback! |
mxcl
force-pushed
the
v7
branch
5 times, most recently
from
c06a298 to
59553d6
Compare
|
My team is considering incorporating these cancel features into one or more of our apps. Is there anyway we can pull this version into our app using Cocoapods to start playing around with it and giving you guys some feedback? |
That would be useful thanks! You can specify branches for a pod spec, though currently this is not merging. Though that shouldn't matter since you can specify @dougzilla32’s fork and branch. |
We'll alpha v7 after merging and I'll test it out in all my apps, so we can figure out changes to names etc. then. |
…unction to 'cancellize'
|
I merged in the latest 'v7' changes and renamed the 'cancellable' function to 'cancellize'. |
|
I took a look at Doug's clone just to see how much the V7 Dispatcher stuff was muddying the waters for this kind of change. I didn't run any code, but the integration looks correct and complete. Nice work! It's a shame to have to write these "backward compatibility" wrappers for new code, but it seems to be the only plausible way to continue to support nice forms like An unrelated comment: as someone new to the cancellation API, it seems a bit strange to me that I wonder about Maybe it's worth biting the bullet and going with something clunky but clear like Why is the user-facing cancellation protocol |
|
Thanks!! I tried to be thorough thinking we can easily delete boilerplate/wrappers later if they aren't really needed. Should the backwards compatible wrappers be marked as deprecated? I don't know enough about the trade-off to have an opinion here, just lmk what makes the most sense and I can update the code. Great point about For If cancellation were a method on cancellizeFirstly {
Promise.value(1)
}.then { x in
Promise.value(2 + x)
}.then { x in
Promise.value(3 + x)
}
firstly {
Promise.value(1) // <-- not cancellable
}.cancellizeThen { x in
Promise.value(2 + x)
}.then { x in
Promise.value(3 + x)
}or cancellizeFirstly {
Promise.value(1)
}.cancellizeThen { x in
Promise.value(2 + x)
}.cancellizeThen { x in
Promise.value(3 + x)
}
firstly {
Promise.value(1) // <-- not cancellable
}.cancellizeThen { x in
Promise.value(2 + x)
}.cancellizeThen { x in
Promise.value(3 + x)
}vs how it currently is: firstly {
cancellize(Promise.value(1))
).then { x in
cancellize(Promise.value(2 + x))
).then { x in
cancellize(Promise.value(3 + x))
}
firstly {
Promise.value(1) // <-- not cancellable
).then { x in
cancellize(Promise.value(2 + x))
).then { x in
cancellize(Promise.value(3 + x))
}or // Could be changed to something like this
firstly {
cancellize(Promise.value(1))
).then { x in
Promise.value(2 + x)
).then { x in
Promise.value(3 + x)
}
firstly {
Promise.value(1) // <-- not cancellable
).then { x in
cancellize(Promise.value(2 + x))
).then { x in
Promise.value(3 + x)
}I don't have a strong opinion on this one. Thank you for taking the time to give this insightful feedback!! |
What you have looks just right as it is. I can do a more detailed pass through once this is integrated, but it's probably not going to turn up much. I put "backward compatibility" in quotes because the wrappers are really just there to allow the existing API to continue forward into the future -- not (only) because that API is already being used, but because it's clearer and more concise than a Dispatcher-only API would be. Dispatcher is a protocol, and protocols can't have static members, so there's no way to allow Regarding cancellize(Promise.value(2 + x))rather than Promise.value(2 + x).cancellize()Of course, the distinction is largely stylistic. But aside from not arousing the attention of the Free Function Police, a method on Thenable would allow you use Promise.value(2 + x).cancellize().then { ... }Is it wrong of me to think of cancellization this way? I guess I'm thinking more broadly that if head-to-toe-cancellable chains are the recommended happy path, shouldn't CancellablePromise's firstly {
cancellize(Promise.value(1))
).then { x in
cancellize(Promise.value(2 + x))
).then { x in
cancellize(Promise.value(3 + x))
}as shown above, why not firstly {
Promise.value(1)
).cancellize().then { x in
Promise.value(2 + x)
).then { x in
Promise.value(3 + x)
} |
…' function to 'Thenable.cancellize' method
|
Garth, your suggestion is excellent!! Just as you suggested I tried moving the global function It now works exactly as you suggest above. As a bonus I eliminated all the Max, if there's a good reason to have it as a global function I can back it out. Just lmk. |
|
I'm going to merge so this doesn’t get out of sync again, which is painful for @dougzilla32 Sorry it took so long, I wanted to do a thorough review, but it's a big patch and I couldn't get my head around it properly in a PR. So let’s try it out in real code and see what we think. Thanks everyone! |
|
Wow, awesome!! |
|
Great! I'm glad the comments were helpful. I'm looking forward to trying this out! |
This patch adds the concept of a `CancelContext` to PromiseKit allowing chains to handle cancellation properly for each independent promise after the cancel point.
This patch adds the concept of a `CancelContext` to PromiseKit allowing chains to handle cancellation properly for each independent promise after the cancel point.
These are the diffs for option 1 of Proposal for PromiseKit cancellation support #896. With option 1 the new cancellation code is included with CorePromise.
The repositories involved with the pull request for option 1 are: