feat: support cancellation for long running operations #160
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
- Coverage 95.66% 95.59% -0.08%
==========================================
Files 83 83
Lines 3900 3947 +47
==========================================
+ Hits 3731 3773 +42
- Misses 169 174 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is really good, a few suggestions to reduce the code duplication and some questions around how to report and handle errors.
Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @tmatsuo)
google/cloud/future_generic.h, line 82 at r1 (raw file):
* Cancel the future by invoking cancel() on the shared state. */ void cancel() { this->shared_state_->cancel(); }
It seems like this function can be refactored to future_base<T>?
What is the desired behavior if somebody tries to cancel a satisfied future? I would find it surprising for the cancel request to be successful in that case. Maybe it should fail? More on how to do this later.
google/cloud/future_generic.h, line 182 at r1 (raw file):
* Returns if it is "cancelled". */ bool cancelled() { return this->shared_state_->cancelled_; }
I think a more interesting accessor is cancellable() which should return false for any promise that is satisfied or that has been cancelled already. I think we should refactor that behavior to shared_state_.
google/cloud/future_generic_test.cc, line 638 at r1 (raw file):
// this is just giving implementors freedom. /// @test Verify the behavior around cancellation.
I think we should also have tests for canceling a future that is already satisfied, and satisfying a future that has a cancel request on it. The first should fail (with an error TBD) the second should succeed.
google/cloud/future_void.h, line 79 at r1 (raw file):
* Cancel the future by invoking cancel() on the shared state. */ void cancel() { shared_state_->cancel(); }
Same comments as above re: refactoring to future_base
google/cloud/future_void.h, line 178 at r1 (raw file):
* Returns if it is "cancelled". */ bool cancelled() { return shared_state_->cancelled_; }
Same comments as above re: cancellable()
google/cloud/future_void_test.cc, line 647 at r1 (raw file):
// this is just giving implementors freedom. /// @test Verify the behavior around cancellation.
Same comments as above re: additional tests.
google/cloud/internal/future_base.h, line 147 at r1 (raw file):
class promise_base { public: promise_base(std::function<void()> cancellation_callback = [] {})
We have the default callback specified in two places, consider using an explicit default constructor for this class.
google/cloud/internal/future_impl.h, line 71 at r1 (raw file):
class future_shared_state_base { public: future_shared_state_base(std::function<void()> cancellation_callback = [] {})
You can also do:
future_shared_state_base() : future_shared_state_base([]{}) {}Which might be more readable?
google/cloud/internal/future_impl.h, line 208 at r1 (raw file):
} cancellation_callback_(); cancelled_ = true;
This is not set if an exception is thrown by cancellation_callback_(). That might or might not be the desired behavior. I think it is: if the cancel callback throws then it could not "do its job" which should be "request a cancellation". Maybe a comment explaining this decision?
google/cloud/internal/future_impl.h, line 262 at r1 (raw file):
} // My (@coryan) reading of the spec is that calling get_future() on a promise
BTW, here is another reason to keep the cancelled_ flag in the shared state, I had forgotten about it.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @coryan)
google/cloud/future_generic.h, line 82 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
It seems like this function can be refactored to
future_base<T>?What is the desired behavior if somebody tries to cancel a satisfied future? I would find it surprising for the cancel request to be successful in that case. Maybe it should fail? More on how to do this later.
How about to return bool?
google/cloud/future_generic.h, line 182 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think a more interesting accessor is
cancellable()which should returnfalsefor any promise that is satisfied or that has been cancelled already. I think we should refactor that behavior toshared_state_.
Done
google/cloud/future_generic_test.cc, line 638 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think we should also have tests for canceling a future that is already satisfied, and satisfying a future that has a cancel request on it. The first should fail (with an error TBD) the second should succeed.
Done
google/cloud/future_void.h, line 79 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Same comments as above re: refactoring to
future_base
Done
google/cloud/future_void.h, line 178 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Same comments as above re:
cancellable()
Done
google/cloud/future_void_test.cc, line 647 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Same comments as above re: additional tests.
Done
google/cloud/internal/future_base.h, line 147 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
We have the default callback specified in two places, consider using an explicit default constructor for this class.
Done
google/cloud/internal/future_impl.h, line 71 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
You can also do:
future_shared_state_base() : future_shared_state_base([]{}) {}Which might be more readable?
Done
google/cloud/internal/future_impl.h, line 208 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This is not set if an exception is thrown by
cancellation_callback_(). That might or might not be the desired behavior. I think it is: if the cancel callback throws then it could not "do its job" which should be "request a cancellation". Maybe a comment explaining this decision?
I introduced try and catch, also added some comments
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
- Coverage 95.66% 95.63% -0.03%
==========================================
Files 83 83
Lines 3900 3944 +44
==========================================
+ Hits 3731 3772 +41
- Misses 169 172 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think I liked cancel() better before. Thoughts?
Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tmatsuo)
google/cloud/future_generic_test.cc, line 648 at r2 (raw file):
/// @test Verify the behavior around cancellation. TEST(FutureTestInt, cancellation_and_sdatisfaction) {
nit: typo is sdatis -> satis
google/cloud/internal/future_impl.h, line 208 at r1 (raw file):
Previously, tmatsuo (Takashi Matsuo) wrote…
I introduced try and catch, also added some comments
I liked it better before: less code and more information returned to the application. Returning a bool seems like a good idea, so what do you think of this code:
bool cancel() {
if (!cancellable()) return false;
cancellation_callback_();
// If the callback fails with an exception we assume it had no effect.
// Incidentally this means we provide the strong exception guarantee for this function.
cancelled_ = true;
return true;
}
There was a problem hiding this comment.
Sure, PTAL
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @coryan)
google/cloud/internal/future_impl.h, line 208 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I liked it better before: less code and more information returned to the application. Returning a
boolseems like a good idea, so what do you think of this code:bool cancel() { if (!cancellable()) return false; cancellation_callback_(); // If the callback fails with an exception we assume it had no effect. // Incidentally this means we provide the strong exception guarantee for this function. cancelled_ = true; return true; }
I somehow thought differently, but now I agree.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
| @@ -279,6 +299,10 @@ class future_shared_state_base { | |||
| * member variable and does not satisfy the shared state. | |||
| */ | |||
| std::unique_ptr<continuation_base> continuation_; | |||
|
|
|||
| // Allow users "cancel" the future with the given callback. | |||
| std::atomic<bool> cancelled_ = ATOMIC_VAR_INIT(false); | |||
There was a problem hiding this comment.
Why need this be atomic? Or, more generally, what are the concurrency requirements of cancel()? Must callers serialize cancel() calls? If so, atomicity doesn't seem to be required. If not, atomicity doesn't seem sufficient.
There was a problem hiding this comment.
@devbww Good point. Maybe I can change it to bool. It is sufficient for my use case.
…ogle-cloud-cpp-common#160) * feat: support cancellation for long running operations * address review comments * address code review comments
…ogle-cloud-cpp-common#160) * feat: support cancellation for long running operations * address review comments * address code review comments
…ogle-cloud-cpp-common#160) * feat: support cancellation for long running operations * address review comments * address code review comments
This change is