feat!: change the result type for timers to StatusOr

google/cloud/grpc_utils/completion_queue.cc

@@ -41,7 +41,7 @@ class AsyncTimerFuture : public internal::AsyncGrpcOperation {
   explicit AsyncTimerFuture(std::unique_ptr<grpc::Alarm> alarm)
       : alarm_(std::move(alarm)) {}
 
-  future<std::chrono::system_clock::time_point> GetFuture() {
+  future<StatusOr<std::chrono::system_clock::time_point>> 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<std::chrono::system_clock::time_point> promise_;
+  promise<StatusOr<std::chrono::system_clock::time_point>> promise_;
   std::chrono::system_clock::time_point deadline_;
   /// Holds the underlying handle. It might be a nullptr in tests.
   std::unique_ptr<grpc::Alarm> alarm_;
@@ -82,7 +86,7 @@ void CompletionQueue::Shutdown() { impl_->Shutdown(); }
 
 void CompletionQueue::CancelAll() { impl_->CancelAll(); }
 
-google::cloud::future<std::chrono::system_clock::time_point>
+google::cloud::future<StatusOr<std::chrono::system_clock::time_point>>
 CompletionQueue::MakeDeadlineTimer(
     std::chrono::system_clock::time_point deadline) {
   auto op = std::make_shared<AsyncTimerFuture>(impl_->CreateAlarm());

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<std::chrono::system_clock::time_point>
+  google::cloud::future<StatusOr<std::chrono::system_clock::time_point>>
   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 <typename Rep, typename Period>
-  future<std::chrono::system_clock::time_point> MakeRelativeTimer(
+  future<StatusOr<std::chrono::system_clock::time_point>> MakeRelativeTimer(
       std::chrono::duration<Rep, Period> duration) {
     return MakeDeadlineTimer(std::chrono::system_clock::now() + duration);
   }
@@ -175,7 +179,10 @@ class CompletionQueue {
                 internal::CheckRunAsyncCallback<Functor>::value, int>::type = 0>
   void RunAsync(Functor&& functor) {
     MakeRelativeTimer(std::chrono::seconds(0))
-        .then([this, functor](future<std::chrono::system_clock::time_point>) {
+        .then([this, functor](
+                  future<StatusOr<std::chrono::system_clock::time_point>>) {
+          // We intentionally ignore the status here; the functor is always
+          // called, even after a call to `CancelAll`.
           CompletionQueue cq(impl_);
           functor(cq);
         });

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 <gtest/gtest.h>
 #include <chrono>
 #include <memory>
@@ -38,7 +39,8 @@ TEST(CompletionQueueTest, TimerSmokeTest) {
   using ms = std::chrono::milliseconds;
   promise<void> wait_for_sleep;
   cq.MakeRelativeTimer(ms(2))
-      .then([&wait_for_sleep](future<std::chrono::system_clock::time_point>) {
+      .then([&wait_for_sleep](
+                future<StatusOr<std::chrono::system_clock::time_point>>) {
         wait_for_sleep.set_value();
       })
       .get();
@@ -56,7 +58,8 @@ TEST(CompletionQueueTest, MockSmokeTest) {
   using ms = std::chrono::milliseconds;
   promise<void> wait_for_sleep;
   cq.MakeRelativeTimer(ms(20000)).then(
-      [&wait_for_sleep](future<std::chrono::system_clock::time_point>) {
+      [&wait_for_sleep](
+          future<StatusOr<std::chrono::system_clock::time_point>>) {
         wait_for_sleep.set_value();
       });
   mock->SimulateCompletion(cq, true);
@@ -70,11 +73,15 @@ TEST(CompletionQueueTest, ShutdownWithPending) {
   using ms = std::chrono::milliseconds;
 
   future<void> timer;
+  future<void> timer2;
   {
     CompletionQueue cq;
     std::thread runner([&cq] { cq.Run(); });
     timer = cq.MakeRelativeTimer(ms(20)).then(
-        [](future<std::chrono::system_clock::time_point>) {});
+        [](future<StatusOr<std::chrono::system_clock::time_point>> 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 +99,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<StatusOr<std::chrono::system_clock::time_point>> 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();