feat: provide a mockable Clock class (#1452)

* feat: provide a mockable `Clock` class

Like #1449 but different; this should make it easier to add operations
in the future (like sleep/wait), at the cost of having to pass it around
as a `shared_ptr` even for the `RealClock`.  (In #1449 we only needed
the `shared_ptr` in the fake case.)

fixes #1428

* address review comments

* address review comments

* address review comments

* fix include guards after #1456

* API update, again

Author: mr-salty

ci/test-api/spanner_client.expected.abi.dump.gz

@@ -131,6 +131,7 @@ add_library(
     internal/api_client_header.h
     internal/build_info.h
     internal/channel.h
+    internal/clock.h
     internal/compiler_info.cc
     internal/compiler_info.h
     internal/connection_impl.cc
@@ -278,6 +279,7 @@ function (spanner_client_define_tests)
         testing/compiler_supports_regexp.h
         testing/database_environment.cc
         testing/database_environment.h
+        testing/fake_clock.h
         testing/matchers.h
         testing/mock_database_admin_stub.h
         testing/mock_instance_admin_stub.h
@@ -328,6 +330,7 @@ function (spanner_client_define_tests)
         instance_test.cc
         internal/api_client_header_test.cc
         internal/build_info_test.cc
+        internal/clock_test.cc
         internal/compiler_info_test.cc
         internal/connection_impl_test.cc
         internal/database_admin_logging_test.cc

google/cloud/spanner/CMakeLists.txt

@@ -0,0 +1,65 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_SPANNER_INTERNAL_CLOCK_H
+#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_SPANNER_INTERNAL_CLOCK_H
+
+#include "google/cloud/spanner/version.h"
+#include <chrono>
+
+namespace google {
+namespace cloud {
+namespace spanner {
+inline namespace SPANNER_CLIENT_NS {
+namespace internal {
+
+/**
+ * A simple `Clock` class that can be overridden for testing.
+ *
+ * All implementations of this class are required to be thread-safe.
+ *
+ * The template type `TrivialClock` must meet the C++ named requirements for
+ * `TrivialClock` (for example, clocks from `std::chrono`).
+ */
+template <typename TrivialClock>
+class Clock {
+ public:
+  using time_point = typename TrivialClock::time_point;
+  using duration = typename TrivialClock::duration;
+
+  virtual ~Clock() = default;
+  virtual time_point Now() const { return TrivialClock::now(); }
+};
+
+/**
+ * `SteadyClock` is a monotonic clock where time points cannot decrease as
+ * physical time moves forward. It is not related to wall clock time.
+ */
+using SteadyClock =
+    ::google::cloud::spanner::internal::Clock<std::chrono::steady_clock>;
+
+/**
+ * `SystemClock` represents the system-wide real time wall clock.
+ * It may not be monotonic.
+ */
+using SystemClock =
+    ::google::cloud::spanner::internal::Clock<std::chrono::system_clock>;
+
+}  // namespace internal
+}  // namespace SPANNER_CLIENT_NS
+}  // namespace spanner
+}  // namespace cloud
+}  // namespace google
+
+#endif  // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_SPANNER_INTERNAL_CLOCK_H

google/cloud/spanner/internal/clock.h

@@ -0,0 +1,70 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "google/cloud/spanner/internal/clock.h"
+#include "google/cloud/spanner/testing/fake_clock.h"
+#include <gmock/gmock.h>
+#include <memory>
+
+namespace google {
+namespace cloud {
+namespace spanner {
+inline namespace SPANNER_CLIENT_NS {
+namespace internal {
+namespace {
+
+using ::google::cloud::spanner_testing::FakeClock;
+
+TEST(Clock, SteadyClock) {
+  auto clock = std::make_shared<SteadyClock>();
+  auto now = clock->Now();
+  auto now2 = clock->Now();
+  // `SteadyClock::Now()` can never decrease as physical time moves forward.
+  EXPECT_LE(now, now2);
+}
+
+TEST(Clock, SystemClock) {
+  auto clock = std::make_shared<SystemClock>();
+  // There is no guarantee that `SystemClock::Now()` can never decrease, so
+  // we can't test that like we do for `SteadyClock`, so for now just make
+  // sure `Now()` is callable.
+  (void)clock->Now();
+}
+
+TEST(Clock, FakeClock) {
+  SteadyClock real_clock;
+  FakeClock<SteadyClock> clock;
+  SteadyClock::time_point time(real_clock.Now());
+  clock.SetTime(time);
+  EXPECT_EQ(clock.Now(), time);
+
+  time += std::chrono::minutes(3);
+  clock.SetTime(time);
+  EXPECT_EQ(clock.Now(), time);
+
+  SteadyClock::duration duration = std::chrono::hours(89);
+  time += duration;
+  clock.AdvanceTime(duration);
+  EXPECT_EQ(clock.Now(), time);
+  time += duration;
+  clock.AdvanceTime(duration);
+  EXPECT_EQ(clock.Now(), time);
+}
+
+}  // namespace
+}  // namespace internal
+}  // namespace SPANNER_CLIENT_NS
+}  // namespace spanner
+}  // namespace cloud
+}  // namespace google

google/cloud/spanner/internal/clock_test.cc

@@ -16,6 +16,7 @@
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_SPANNER_INTERNAL_SESSION_H
 
 #include "google/cloud/spanner/internal/channel.h"
+#include "google/cloud/spanner/internal/clock.h"
 #include "google/cloud/spanner/version.h"
 #include <atomic>
 #include <chrono>
@@ -36,11 +37,14 @@ namespace internal {
  */
 class Session {
  public:
-  Session(std::string session_name, std::shared_ptr<Channel> channel)
+  using Clock = ::google::cloud::spanner::internal::SteadyClock;
+  Session(std::string session_name, std::shared_ptr<Channel> channel,
+          std::shared_ptr<Clock> clock = std::make_shared<Clock>())
       : session_name_(std::move(session_name)),
         channel_(std::move(channel)),
         is_bad_(false),
-        last_use_time_(Clock::now()) {}
+        clock_(std::move(clock)),
+        last_use_time_(clock_->Now()) {}
 
   // Not copyable or moveable.
   Session(Session const&) = delete;
@@ -55,19 +59,19 @@ class Session {
   bool is_bad() const { return is_bad_.load(std::memory_order_relaxed); }
 
  private:
-  // Give `SessionPool` access to the private types/methods below.
+  // Give `SessionPool` access to the private methods below.
   friend class SessionPool;
-  using Clock = std::chrono::steady_clock;
   std::shared_ptr<Channel> const& channel() const { return channel_; }
 
   // The caller is responsible for ensuring these methods are used in a
   // thread-safe manner (i.e. using external locking).
   Clock::time_point last_use_time() const { return last_use_time_; }
-  void update_last_use_time() { last_use_time_ = Clock::now(); }
+  void update_last_use_time() { last_use_time_ = clock_->Now(); }
 
   std::string const session_name_;
   std::shared_ptr<Channel> const channel_;
   std::atomic<bool> is_bad_;
+  std::shared_ptr<Clock> clock_;
   Clock::time_point last_use_time_;
 };
 

google/cloud/spanner/internal/session.h

@@ -40,10 +40,11 @@ std::shared_ptr<SessionPool> MakeSessionPool(
     Database db, std::vector<std::shared_ptr<SpannerStub>> stubs,
     SessionPoolOptions options, google::cloud::CompletionQueue cq,
     std::unique_ptr<RetryPolicy> retry_policy,
-    std::unique_ptr<BackoffPolicy> backoff_policy) {
+    std::unique_ptr<BackoffPolicy> backoff_policy,
+    std::shared_ptr<Session::Clock> clock) {
   auto pool = std::make_shared<SessionPool>(
       std::move(db), std::move(stubs), std::move(options), std::move(cq),
-      std::move(retry_policy), std::move(backoff_policy));
+      std::move(retry_policy), std::move(backoff_policy), std::move(clock));
   pool->Initialize();
   return pool;
 }
@@ -53,13 +54,15 @@ SessionPool::SessionPool(Database db,
                          SessionPoolOptions options,
                          google::cloud::CompletionQueue cq,
                          std::unique_ptr<RetryPolicy> retry_policy,
-                         std::unique_ptr<BackoffPolicy> backoff_policy)
+                         std::unique_ptr<BackoffPolicy> backoff_policy,
+                         std::shared_ptr<Session::Clock> clock)
     : db_(std::move(db)),
       options_(std::move(
           options.EnforceConstraints(static_cast<int>(stubs.size())))),
       cq_(std::move(cq)),
       retry_policy_prototype_(std::move(retry_policy)),
       backoff_policy_prototype_(std::move(backoff_policy)),
+      clock_(std::move(clock)),
       max_pool_size_(options_.max_sessions_per_channel() *
                      static_cast<int>(stubs.size())),
       random_generator_(std::random_device()()) {
@@ -135,15 +138,14 @@ void SessionPool::MaintainPoolSize() {
 void SessionPool::RefreshExpiringSessions() {
   std::vector<std::pair<std::shared_ptr<SpannerStub>, std::string>>
       sessions_to_refresh;
-  Session::Clock::time_point now = Session::Clock::now();
-  Session::Clock::time_point refresh_limit =
-      now - options_.keep_alive_interval();
+  auto now = clock_->Now();
+  auto refresh_limit = now - options_.keep_alive_interval();
   {
     std::unique_lock<std::mutex> lk(mu_);
     if (last_use_time_lower_bound_ <= refresh_limit) {
       last_use_time_lower_bound_ = now;
       for (auto const& session : sessions_) {
-        Session::Clock::time_point last_use_time = session->last_use_time();
+        auto last_use_time = session->last_use_time();
         if (last_use_time <= refresh_limit) {
           sessions_to_refresh.emplace_back(session->channel()->stub,
                                            session->session_name());
@@ -474,7 +476,7 @@ Status SessionPool::HandleBatchCreateSessionsDone(
   sessions_.reserve(sessions_.size() + sessions_created);
   for (auto& session : *response->mutable_session()) {
     sessions_.push_back(google::cloud::internal::make_unique<Session>(
-        std::move(*session.mutable_name()), channel));
+        std::move(*session.mutable_name()), channel, clock_));
   }
   // Shuffle the pool so we distribute returned sessions across channels.
   std::shuffle(sessions_.begin(), sessions_.end(), random_generator_);

google/cloud/spanner/internal/session_pool.cc

@@ -69,7 +69,8 @@ class SessionPool : public std::enable_shared_from_this<SessionPool> {
   SessionPool(Database db, std::vector<std::shared_ptr<SpannerStub>> stubs,
               SessionPoolOptions options, google::cloud::CompletionQueue cq,
               std::unique_ptr<RetryPolicy> retry_policy,
-              std::unique_ptr<BackoffPolicy> backoff_policy);
+              std::unique_ptr<BackoffPolicy> backoff_policy,
+              std::shared_ptr<Session::Clock> clock);
 
   ~SessionPool();
 
@@ -165,6 +166,7 @@ class SessionPool : public std::enable_shared_from_this<SessionPool> {
   google::cloud::CompletionQueue cq_;
   std::unique_ptr<RetryPolicy const> retry_policy_prototype_;
   std::unique_ptr<BackoffPolicy const> backoff_policy_prototype_;
+  std::shared_ptr<Session::Clock> clock_;
   int const max_pool_size_;
   std::mt19937 random_generator_;
 
@@ -177,7 +179,7 @@ class SessionPool : public std::enable_shared_from_this<SessionPool> {
 
   // Lower bound on all `sessions_[i]->last_use_time()` values.
   Session::Clock::time_point last_use_time_lower_bound_ =
-      Session::Clock::now();  // GUARDED_BY(mu_)
+      clock_->Now();  // GUARDED_BY(mu_)
 
   future<void> current_timer_;
 
@@ -200,7 +202,8 @@ std::shared_ptr<SessionPool> MakeSessionPool(
     Database db, std::vector<std::shared_ptr<SpannerStub>> stubs,
     SessionPoolOptions options, google::cloud::CompletionQueue cq,
     std::unique_ptr<RetryPolicy> retry_policy,
-    std::unique_ptr<BackoffPolicy> backoff_policy);
+    std::unique_ptr<BackoffPolicy> backoff_policy,
+    std::shared_ptr<Session::Clock> clock = std::make_shared<Session::Clock>());
 
 }  // namespace internal
 }  // namespace SPANNER_CLIENT_NS

google/cloud/spanner/internal/session_pool.h

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 #include "google/cloud/spanner/internal/session_pool.h"
+#include "google/cloud/spanner/internal/clock.h"
 #include "google/cloud/spanner/internal/session.h"
+#include "google/cloud/spanner/testing/fake_clock.h"
 #include "google/cloud/spanner/testing/mock_spanner_stub.h"
 #include "google/cloud/internal/background_threads_impl.h"
 #include "google/cloud/internal/make_unique.h"
@@ -35,6 +37,7 @@ inline namespace SPANNER_CLIENT_NS {
 namespace internal {
 namespace {
 
+using ::google::cloud::spanner_testing::FakeSteadyClock;
 using ::google::cloud::testing_util::MockAsyncResponseReader;
 using ::google::cloud::testing_util::MockCompletionQueue;
 using ::testing::_;
@@ -71,13 +74,15 @@ spanner_proto::BatchCreateSessionsResponse MakeSessionsResponse(
 
 std::shared_ptr<SessionPool> MakeSessionPool(
     Database db, std::vector<std::shared_ptr<SpannerStub>> stubs,
-    SessionPoolOptions options, CompletionQueue cq) {
+    SessionPoolOptions options, CompletionQueue cq,
+    std::shared_ptr<SteadyClock> clock = std::make_shared<SteadyClock>()) {
   return MakeSessionPool(
       std::move(db), std::move(stubs), std::move(options), std::move(cq),
       google::cloud::internal::make_unique<LimitedTimeRetryPolicy>(
           std::chrono::minutes(10)),
       google::cloud::internal::make_unique<ExponentialBackoffPolicy>(
-          std::chrono::milliseconds(100), std::chrono::minutes(1), 2.0));
+          std::chrono::milliseconds(100), std::chrono::minutes(1), 2.0),
+      std::move(clock));
 }
 
 TEST(SessionPool, Allocate) {
@@ -388,9 +393,6 @@ TEST(SessionPool, GetStubForStublessSession) {
   EXPECT_EQ(pool->GetStub(*session), mock);
 }
 
-// TODO(#1428): This test has a real-time component (see sleep_for() below)
-// as SessionPool does not currently provide a mechanism to inject a clock
-// source to control whether sessions require refreshing.
 TEST(SessionPool, SessionRefresh) {
   auto mock = std::make_shared<StrictMock<spanner_testing::MockSpannerStub>>();
   EXPECT_CALL(*mock, BatchCreateSessions(_, _))
@@ -421,7 +423,9 @@ TEST(SessionPool, SessionRefresh) {
   SessionPoolOptions options;
   options.set_keep_alive_interval(std::chrono::seconds(1));
   auto impl = std::make_shared<MockCompletionQueue>();
-  auto pool = MakeSessionPool(db, {mock}, options, CompletionQueue(impl));
+  auto clock = std::make_shared<FakeSteadyClock>();
+  auto pool =
+      MakeSessionPool(db, {mock}, options, CompletionQueue(impl), clock);
 
   // Allocate and release two session, "s1" and "s2". This will satisfy the
   // BatchCreateSessions() expectations.
@@ -435,7 +439,7 @@ TEST(SessionPool, SessionRefresh) {
       EXPECT_EQ("s2", (*s2)->session_name());
     }
     // Wait for "s2" to need refreshing before releasing "s1".
-    std::this_thread::sleep_for(options.keep_alive_interval() * 2);
+    clock->AdvanceTime(options.keep_alive_interval() * 2);
   }
 
   // Simulate completion of pending operations, which will result in

google/cloud/spanner/internal/session_pool_test.cc

@@ -38,6 +38,7 @@ spanner_client_hdrs = [
     "internal/api_client_header.h",
     "internal/build_info.h",
     "internal/channel.h",
+    "internal/clock.h",
     "internal/compiler_info.h",
     "internal/connection_impl.h",
     "internal/database_admin_logging.h",

google/cloud/spanner/spanner_client.bzl

@@ -20,6 +20,7 @@ spanner_client_testing_hdrs = [
     "testing/cleanup_stale_instances.h",
     "testing/compiler_supports_regexp.h",
     "testing/database_environment.h",
+    "testing/fake_clock.h",
     "testing/matchers.h",
     "testing/mock_database_admin_stub.h",
     "testing/mock_instance_admin_stub.h",