From 7742997c481cfbe68e09d1411f3d003632342e39 Mon Sep 17 00:00:00 2001 From: Todd Derr Date: Mon, 27 Jan 2020 15:46:15 -0500 Subject: [PATCH] feat!: change the result type for timers to StatusOr (googleapis/google-cloud-cpp-common#134) The result of the `future` returned from `MakeDeadlineTimer` and`MakeRelativeTimer` is now a `StatusOr` that indicates whether the timer ran to expiration or not (e.g. it was cancelled). Part of googleapis/google-cloud-cpp-common#129 --- google/cloud/grpc_utils/completion_queue.cc | 14 +++++++----- google/cloud/grpc_utils/completion_queue.h | 13 ++++++++--- .../cloud/grpc_utils/completion_queue_test.cc | 22 ++++++++++++++----- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/google/cloud/grpc_utils/completion_queue.cc b/google/cloud/grpc_utils/completion_queue.cc index ab2017f8a4a1a..c48c12f756a54 100644 --- a/google/cloud/grpc_utils/completion_queue.cc +++ b/google/cloud/grpc_utils/completion_queue.cc @@ -41,7 +41,7 @@ class AsyncTimerFuture : public internal::AsyncGrpcOperation { explicit AsyncTimerFuture(std::unique_ptr alarm) : alarm_(std::move(alarm)) {} - future GetFuture() { + future> GetFuture() { return promise_.get_future(); } @@ -61,12 +61,16 @@ class AsyncTimerFuture : public internal::AsyncGrpcOperation { } private: - bool Notify(CompletionQueue&, bool) override { - promise_.set_value(deadline_); + bool Notify(CompletionQueue&, bool ok) override { + if (!ok) { + promise_.set_value(Status(StatusCode::kCancelled, "timer canceled")); + } else { + promise_.set_value(deadline_); + } return true; } - promise promise_; + promise> promise_; std::chrono::system_clock::time_point deadline_; /// Holds the underlying handle. It might be a nullptr in tests. std::unique_ptr alarm_; @@ -82,7 +86,7 @@ void CompletionQueue::Shutdown() { impl_->Shutdown(); } void CompletionQueue::CancelAll() { impl_->CancelAll(); } -google::cloud::future +google::cloud::future> CompletionQueue::MakeDeadlineTimer( std::chrono::system_clock::time_point deadline) { auto op = std::make_shared(impl_->CreateAlarm()); diff --git a/google/cloud/grpc_utils/completion_queue.h b/google/cloud/grpc_utils/completion_queue.h index 3993f32ba94f2..f185efd9b0279 100644 --- a/google/cloud/grpc_utils/completion_queue.h +++ b/google/cloud/grpc_utils/completion_queue.h @@ -54,8 +54,10 @@ class CompletionQueue { * @param deadline when should the timer expire. * * @return a future that becomes satisfied after @p deadline. + * The result of the future is the time at which it expired, or an error + * Status if the timer did not run to expiration (e.g. it was cancelled). */ - google::cloud::future + google::cloud::future> MakeDeadlineTimer(std::chrono::system_clock::time_point deadline); /** @@ -75,9 +77,11 @@ class CompletionQueue { * @param duration when should the timer expire relative to the current time. * * @return a future that becomes satisfied after @p duration time has elapsed. + * The result of the future is the time at which it expired, or an error + * Status if the timer did not run to expiration (e.g. it was cancelled). */ template - future MakeRelativeTimer( + future> MakeRelativeTimer( std::chrono::duration duration) { return MakeDeadlineTimer(std::chrono::system_clock::now() + duration); } @@ -175,7 +179,10 @@ class CompletionQueue { internal::CheckRunAsyncCallback::value, int>::type = 0> void RunAsync(Functor&& functor) { MakeRelativeTimer(std::chrono::seconds(0)) - .then([this, functor](future) { + .then([this, functor]( + future>) { + // We intentionally ignore the status here; the functor is always + // called, even after a call to `CancelAll`. CompletionQueue cq(impl_); functor(cq); }); diff --git a/google/cloud/grpc_utils/completion_queue_test.cc b/google/cloud/grpc_utils/completion_queue_test.cc index a3ead93e67bc5..2320769de1ea9 100644 --- a/google/cloud/grpc_utils/completion_queue_test.cc +++ b/google/cloud/grpc_utils/completion_queue_test.cc @@ -14,6 +14,7 @@ #include "google/cloud/grpc_utils/completion_queue.h" #include "google/cloud/future.h" +#include "google/cloud/testing_util/assert_ok.h" #include #include #include @@ -38,7 +39,8 @@ TEST(CompletionQueueTest, TimerSmokeTest) { using ms = std::chrono::milliseconds; promise wait_for_sleep; cq.MakeRelativeTimer(ms(2)) - .then([&wait_for_sleep](future) { + .then([&wait_for_sleep]( + future>) { wait_for_sleep.set_value(); }) .get(); @@ -56,7 +58,8 @@ TEST(CompletionQueueTest, MockSmokeTest) { using ms = std::chrono::milliseconds; promise wait_for_sleep; cq.MakeRelativeTimer(ms(20000)).then( - [&wait_for_sleep](future) { + [&wait_for_sleep]( + future>) { wait_for_sleep.set_value(); }); mock->SimulateCompletion(cq, true); @@ -74,7 +77,10 @@ TEST(CompletionQueueTest, ShutdownWithPending) { CompletionQueue cq; std::thread runner([&cq] { cq.Run(); }); timer = cq.MakeRelativeTimer(ms(20)).then( - [](future) {}); + [](future> result) { + // Timer still runs to completion after `Shutdown`. + EXPECT_STATUS_OK(result.get().status()); + }); EXPECT_EQ(std::future_status::timeout, timer.wait_for(ms(0))); cq.Shutdown(); EXPECT_EQ(std::future_status::timeout, timer.wait_for(ms(0))); @@ -92,9 +98,13 @@ TEST(CompletionQueueTest, CanCancelAllEvents) { cq.Run(); done.set_value(); }); - cq.MakeRelativeTimer(ms(20000)); - cq.MakeRelativeTimer(ms(20000)); - cq.MakeRelativeTimer(ms(20000)); + for (int i = 0; i < 3; ++i) { + cq.MakeRelativeTimer(ms(20000)).then( + [](future> result) { + // Cancelled timers return CANCELLED status. + EXPECT_EQ(StatusCode::kCancelled, result.get().status().code()); + }); + } auto f = done.get_future(); EXPECT_EQ(std::future_status::timeout, f.wait_for(ms(1))); cq.Shutdown();