address review comments

Author: bajajneha27

google/cloud/storage/internal/bucket_metadata_cache.h

@@ -66,6 +66,19 @@ class BucketMetadataCache {
   std::unordered_set<std::string> in_flight_fetch_;
 };
 
+class ScopedFetch {
+ public:
+  ScopedFetch(BucketMetadataCache* cache, std::string bucket_name)
+      : cache_(cache), bucket_name_(std::move(bucket_name)) {}
+  ~ScopedFetch() {
+    if (cache_) cache_->EndFetch(bucket_name_);
+  }
+
+ private:
+  BucketMetadataCache* cache_;
+  std::string bucket_name_;
+};
+
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage_internal
 }  // namespace cloud

google/cloud/storage/internal/tracing_connection.cc

@@ -30,7 +30,9 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 
 TracingConnection::TracingConnection(std::shared_ptr<StorageConnection> impl,
                                      AsyncRunner runner)
-    : impl_(std::move(impl)), runner_(std::move(runner)) {}
+    : impl_(std::move(impl)),
+      runner_(std::move(runner)),
+      cache_(std::make_shared<BucketMetadataCache>(10000)) {}
 
 TracingConnection::~TracingConnection() = default;
 
@@ -49,10 +51,7 @@ TracingConnection::AsyncRunner const& TracingConnection::runner() {
   return runner_;
 }
 
-BucketMetadataCache& TracingConnection::cache() {
-  static BucketMetadataCache instance(10000);
-  return instance;
-}
+BucketMetadataCache& TracingConnection::cache() const { return *cache_; }
 
 void TracingConnection::ResetCacheForTesting() { cache().Clear(); }
 
@@ -71,18 +70,18 @@ void TracingConnection::MaybeTriggerBackgroundFetch(
   }
 
   auto current_options = google::cloud::internal::SaveCurrentOptions();
-  runner()([this, bucket_name, current_options]() {
+  runner()([impl = impl_, cache = cache_, bucket_name, current_options]() {
+    ScopedFetch guard(cache.get(), bucket_name);
+
     google::cloud::internal::OptionsSpan span(current_options);
     storage::internal::GetBucketMetadataRequest request(bucket_name);
-    auto result = impl_->GetBucketMetadata(request);
+    auto result = impl->GetBucketMetadata(request);
 
     if (result.ok()) {
-      cache().Put(bucket_name, BucketCacheEntry::FromMetadata(*result));
+      cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result));
     } else if (result.status().code() == StatusCode::kPermissionDenied) {
-      cache().Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"});
+      cache->Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"});
     }
-
-    cache().EndFetch(bucket_name);
   });
 }
 

google/cloud/storage/internal/tracing_connection.h

@@ -36,7 +36,7 @@ class TracingConnection : public storage::internal::StorageConnection {
                              AsyncRunner runner = {});
   ~TracingConnection() override;
 
-  static void ResetCacheForTesting();
+  void ResetCacheForTesting();
 
   Options options() const override;
 
@@ -192,25 +192,25 @@ class TracingConnection : public storage::internal::StorageConnection {
                          BucketCacheEntry const& entry);
   void MaybeTriggerBackgroundFetch(std::string const& bucket_name);
 
-  static void MaybeInvalidate(Status const& status,
-                              std::string const& bucket_name) {
+  void MaybeInvalidate(Status const& status, std::string const& bucket_name) {
     if (!status.ok() && status.code() == StatusCode::kNotFound) {
       cache().Invalidate(bucket_name);
     }
   }
 
   template <typename T>
-  static void MaybeInvalidate(StatusOr<T> const& result,
-                              std::string const& bucket_name) {
+  void MaybeInvalidate(StatusOr<T> const& result,
+                       std::string const& bucket_name) {
     MaybeInvalidate(result.status(), bucket_name);
   }
 
-  static BucketMetadataCache& cache();
+  BucketMetadataCache& cache() const;
 
   AsyncRunner const& runner();
 
   std::shared_ptr<StorageConnection> impl_;
   AsyncRunner runner_;
+  std::shared_ptr<BucketMetadataCache> cache_;
   absl::once_flag once_flag_;
 };
 

google/cloud/storage/internal/tracing_connection_test.cc

@@ -65,7 +65,6 @@ TEST(TracingClientTest, Options) {
 }
 
 TEST(TracingClientTest, CustomAsyncRunner) {
-  TracingConnection::ResetCacheForTesting();
   bool invoked = false;
   auto runner = [&invoked](std::function<void()> const& f) {
     invoked = true;
@@ -217,7 +216,6 @@ TEST(TracingClientTest, GetBucketMetadataSuccess) {
 }
 
 TEST(TracingClientTest, BucketMetadataCacheSuccess) {
-  TracingConnection::ResetCacheForTesting();
   auto span_catcher = InstallSpanCatcher();
   auto mock = std::make_shared<MockClient>();
 
@@ -1713,7 +1711,6 @@ TEST(TracingClientTest, DeleteNotification) {
 }
 
 TEST(TracingClientTest, BucketMetadataMaybeInvalidateBucketLevelEvict) {
-  TracingConnection::ResetCacheForTesting();
   auto mock = std::make_shared<MockClient>();
 
   EXPECT_CALL(*mock, options)
@@ -1760,7 +1757,6 @@ TEST(TracingClientTest, BucketMetadataMaybeInvalidateBucketLevelEvict) {
 }
 
 TEST(TracingClientTest, BucketMetadataMaybeInvalidateObjectLevelNoEvict) {
-  TracingConnection::ResetCacheForTesting();
   auto mock = std::make_shared<MockClient>();
 
   EXPECT_CALL(*mock, options)