Fix #1351: Make AprMemCacheTest and RedisCacheTest tests independent from each other

* Add FlushAll() public method to RedisCache and call it right after connecting to server in RedisCacheTest
* Add CacheKeyPrepender wrapper for CacheInterface
* Make AprMemCache add prefixes to all keys added to Memcached. Prefix right now is simply a test case + test name. Flushing Memcached requires us to further modify our apr_memcached fork, which we decided to avoid.

net/instaweb/test.gyp

@@ -276,6 +276,8 @@
         '<(DEPTH)/pagespeed/kernel/base/wildcard_test.cc',
         '<(DEPTH)/pagespeed/kernel/cache/async_cache_test.cc',
         '<(DEPTH)/pagespeed/kernel/cache/cache_batcher_test.cc',
+        '<(DEPTH)/pagespeed/kernel/cache/cache_key_prepender.cc',
+        '<(DEPTH)/pagespeed/kernel/cache/cache_key_prepender_test.cc',
         '<(DEPTH)/pagespeed/kernel/cache/cache_stats_test.cc',
         '<(DEPTH)/pagespeed/kernel/cache/compressed_cache_test.cc',
         '<(DEPTH)/pagespeed/kernel/cache/delay_cache_test.cc',

pagespeed/kernel/cache/cache_key_prepender.cc

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * 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.
+ */
+
+// Author: yeputons@google.com (Egor Suvorov)
+
+#include "pagespeed/kernel/cache/cache_key_prepender.h"
+
+#include "base/logging.h"
+#include "pagespeed/kernel/base/shared_string.h"
+#include "pagespeed/kernel/base/string.h"
+#include "pagespeed/kernel/base/string_util.h"
+#include "pagespeed/kernel/cache/cache_interface.h"
+#include "pagespeed/kernel/cache/delegating_cache_callback.h"
+
+namespace net_instaweb {
+
+CacheKeyPrepender::CacheKeyPrepender(StringPiece prefix, CacheInterface* cache)
+    : cache_(cache), prefix_(prefix) {}
+
+GoogleString CacheKeyPrepender::FormatName(StringPiece prefix,
+                                           StringPiece cache) {
+  return StrCat("KeyPrepender(prefix=", prefix, ",cache=", cache, ")");
+}
+
+class CacheKeyPrepender::KeyPrependerCallback : public DelegatingCacheCallback {
+ public:
+  KeyPrependerCallback(CacheInterface::Callback* callback,
+                       const SharedString& prefix)
+      : DelegatingCacheCallback(callback), prefix_(prefix) {}
+  ~KeyPrependerCallback() override {}
+
+  bool ValidateCandidate(const GoogleString& key,
+                         CacheInterface::KeyState state) override {
+    if (!HasPrefixString(key, prefix_.Value())) {
+      LOG(DFATAL) << "KeyPrependerCallback has received a key without expected "
+                 << "prefix, treating as cache miss";
+      return false;
+    }
+    return DelegatingCacheCallback::ValidateCandidate(
+        key.substr(prefix_.size()), state);
+  }
+
+ private:
+  SharedString prefix_;
+
+  DISALLOW_COPY_AND_ASSIGN(KeyPrependerCallback);
+};
+
+void CacheKeyPrepender::Get(const GoogleString& key, Callback* callback) {
+  // KeyPrependerCallback deletes itself after it's fired.
+  cache_->Get(AddPrefix(key), new KeyPrependerCallback(callback, prefix_));
+}
+
+void CacheKeyPrepender::MultiGet(MultiGetRequest* request) {
+  for (KeyCallback& key_callback : *request) {
+    key_callback.key = AddPrefix(key_callback.key);
+    // KeyPrependerCallback deletes itself after it's fired.
+    key_callback.callback =
+        new KeyPrependerCallback(key_callback.callback, prefix_);
+  }
+  cache_->MultiGet(request);
+}
+
+void CacheKeyPrepender::Put(const GoogleString& key, SharedString* value) {
+  cache_->Put(AddPrefix(key), value);
+}
+
+void CacheKeyPrepender::Delete(const GoogleString& key) {
+  cache_->Delete(AddPrefix(key));
+}
+
+GoogleString CacheKeyPrepender::AddPrefix(const GoogleString& key) {
+  return StrCat(prefix_.Value(), key);
+}
+
+}  // namespace net_instaweb

pagespeed/kernel/cache/cache_key_prepender.h

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * 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.
+ */
+
+// Author: yeputons@google.com (Egor Suvorov)
+
+#ifndef PAGESPEED_KERNEL_CACHE_CACHE_KEY_PREPENDER_H_
+#define PAGESPEED_KERNEL_CACHE_CACHE_KEY_PREPENDER_H_
+
+#include "pagespeed/kernel/base/basictypes.h"
+#include "pagespeed/kernel/base/shared_string.h"
+#include "pagespeed/kernel/base/string.h"
+#include "pagespeed/kernel/base/string_util.h"
+#include "pagespeed/kernel/cache/cache_interface.h"
+
+namespace net_instaweb {
+
+// Implements a cache adapter that prepends a fixed string to all keys that are
+// used in the cache. Can be used for isolating unit tests of external caches
+// (e.g. memcached).
+class CacheKeyPrepender : public CacheInterface {
+ public:
+  // Does not takes ownership of the cache
+  CacheKeyPrepender(StringPiece prefix, CacheInterface* cache);
+  ~CacheKeyPrepender() override {}
+
+  // Implementation of CacheInterface
+  void Get(const GoogleString& key, Callback* callback) override;
+  void MultiGet(MultiGetRequest* request) override;
+  void Put(const GoogleString& key, SharedString* value) override;
+  void Delete(const GoogleString& key) override;
+  CacheInterface* Backend() override { return cache_; }
+  bool IsBlocking() const override { return cache_->IsBlocking(); }
+
+  // Implementation of CacheInterface
+  bool IsHealthy() const override { return cache_->IsHealthy(); }
+  void ShutDown() override { cache_->ShutDown(); }
+  GoogleString Name() const override {
+    return FormatName(prefix_.Value(), cache_->Name());
+  }
+
+  static GoogleString FormatName(StringPiece prefix, StringPiece cache);
+
+ private:
+  class KeyPrependerCallback;
+
+  CacheInterface* cache_;
+  SharedString prefix_;  // copy of prefix to prepend, shared with callbacks
+
+  GoogleString AddPrefix(const GoogleString &key);
+
+  DISALLOW_COPY_AND_ASSIGN(CacheKeyPrepender);
+};
+
+}  // namespace net_instaweb
+
+#endif  // PAGESPEED_KERNEL_CACHE_CACHE_KEY_PREPENDER_H_

pagespeed/kernel/cache/cache_key_prepender_test.cc

@@ -0,0 +1,114 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * 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.
+ */
+// Author: yeputons@google.com (Egor Suvorov)
+
+#include "pagespeed/kernel/cache/cache_key_prepender.h"
+
+#include "pagespeed/kernel/base/basictypes.h"
+#include "pagespeed/kernel/base/gtest.h"
+#include "pagespeed/kernel/base/string.h"
+#include "pagespeed/kernel/cache/cache_interface.h"
+#include "pagespeed/kernel/cache/cache_test_base.h"
+#include "pagespeed/kernel/cache/in_memory_cache.h"
+
+namespace {
+const char kKeyPrefix[] = "Prefix_";
+}  // namespace
+
+namespace net_instaweb {
+
+class CacheKeyPrependerTest : public CacheTestBase {
+ protected:
+  CacheKeyPrependerTest()
+      : backend_cache_(), cache_(kKeyPrefix, &backend_cache_) {}
+
+  CacheInterface* Cache() override { return &cache_; }
+
+  InMemoryCache backend_cache_;
+  CacheKeyPrepender cache_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CacheKeyPrependerTest);
+};
+
+TEST_F(CacheKeyPrependerTest, Get) {
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "Name"), "Value");
+
+  CheckGet("Name", "Value");
+}
+
+TEST_F(CacheKeyPrependerTest, GetNotFound) {
+  CheckPut(cache_.Backend(), "Name", "Value");
+
+  // 'Name' should become 'Prefix_Name' and it's not in backend cache.
+  CheckNotFound("Name");
+}
+
+TEST_F(CacheKeyPrependerTest, Put) {
+  CheckPut("Name", "Value");
+
+  CheckGet(cache_.Backend(), StrCat(kKeyPrefix, "Name"), "Value");
+}
+
+TEST_F(CacheKeyPrependerTest, Delete) {
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "Name"), "Value");
+
+  CheckDelete("Name");
+
+  CheckNotFound("Name");
+}
+
+TEST_F(CacheKeyPrependerTest, MultiGet) {
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "n0"), "v0");
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "n1"), "v1");
+  Callback *n0 = AddCallback();
+  Callback *not_found = AddCallback();
+  Callback *n1 = AddCallback();
+
+  IssueMultiGet(n0, "n0", not_found, "not_found", n1, "n1");
+
+  WaitAndCheck(n0, "v0");
+  WaitAndCheckNotFound(not_found);
+  WaitAndCheck(n1, "v1");
+}
+
+TEST_F(CacheKeyPrependerTest, BasicInvalid) {
+  // Check that we honor callback veto on validity.
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "nameA"), "valueA");
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "nameB"), "valueB");
+  set_invalid_key("nameA");
+
+  CheckNotFound("nameA");
+  CheckGet("nameB", "valueB");
+}
+
+TEST_F(CacheKeyPrependerTest, MultiGetInvalid) {
+  // Check that we honor callback veto on validity in MultiGet.
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "n0"), "v0");
+  CheckPut(cache_.Backend(), StrCat(kKeyPrefix, "n1"), "v1");
+  set_invalid_key("n0");  // should be called before we create any callbacks
+  Callback *n0 = AddCallback();
+  Callback *not_found = AddCallback();
+  Callback *n1 = AddCallback();
+
+  IssueMultiGet(n0, "n0", not_found, "not_found", n1, "n1");
+
+  WaitAndCheckNotFound(n0);
+  WaitAndCheckNotFound(not_found);
+  WaitAndCheck(n1, "v1");
+}
+
+}  // namespace net_instaweb

pagespeed/kernel/cache/cache_test_base.h

@@ -52,6 +52,7 @@ class CacheTestBase : public testing::Test {
       noop_wait_called_ = false;
       value_of_called_when_wait_was_invoked_ = false;
       invalid_value_ = NULL;
+      invalid_key_ = NULL;
       return this;
     }
 
@@ -61,6 +62,9 @@ class CacheTestBase : public testing::Test {
       if ((invalid_value_ != NULL) && (value_str() == invalid_value_)) {
         return false;
       }
+      if ((invalid_key_ != NULL) && (key == invalid_key_)) {
+        return false;
+      }
       return true;
     }
 
@@ -81,6 +85,7 @@ class CacheTestBase : public testing::Test {
     }
 
     void set_invalid_value(const char* v) { invalid_value_ = v; }
+    void set_invalid_key(const char* k) { invalid_key_ = k; }
     StringPiece value_str() { return value()->Value(); }
 
     bool validate_called_;
@@ -90,6 +95,7 @@ class CacheTestBase : public testing::Test {
    private:
     CacheTestBase* test_;
     const char* invalid_value_;
+    const char* invalid_key_;
 
     DISALLOW_COPY_AND_ASSIGN(Callback);
   };
@@ -97,6 +103,7 @@ class CacheTestBase : public testing::Test {
  protected:
   CacheTestBase()
       : invalid_value_(NULL),
+        invalid_key_(NULL),
         mutex_(new NullMutex),
         outstanding_fetches_(0) {
   }
@@ -158,6 +165,7 @@ class CacheTestBase : public testing::Test {
   Callback* AddCallback() {
     Callback* callback = NewCallback();
     callback->set_invalid_value(invalid_value_);
+    callback->set_invalid_key(invalid_key_);
     callbacks_.push_back(callback);
     return callback;
   }
@@ -221,6 +229,7 @@ class CacheTestBase : public testing::Test {
   }
 
   void set_invalid_value(const char* v) { invalid_value_ = v; }
+  void set_invalid_key(const char* k) { invalid_key_ = k; }
 
   // Initiate a cache Get, and return the Callback* which can be
   // passed to WaitAndCheck or WaitAndCheckNotFound.
@@ -260,6 +269,7 @@ class CacheTestBase : public testing::Test {
   }
 
   const char* invalid_value_;  // may be NULL.
+  const char* invalid_key_;  // may be NULL.
   std::vector<Callback*> callbacks_;
   scoped_ptr<AbstractMutex> mutex_;
   int outstanding_fetches_;  // protected by mutex_

pagespeed/system/apr_mem_cache_test.cc

@@ -38,6 +38,8 @@
 #include "pagespeed/kernel/base/string.h"
 #include "pagespeed/kernel/base/string_util.h"
 #include "pagespeed/kernel/base/timer.h"
+#include "pagespeed/kernel/base/posix_timer.h"
+#include "pagespeed/kernel/cache/cache_key_prepender.h"
 #include "pagespeed/kernel/cache/cache_spammer.h"
 #include "pagespeed/kernel/cache/cache_test_base.h"
 #include "pagespeed/kernel/cache/fallback_cache.h"
@@ -96,7 +98,21 @@ class AprMemCacheTest : public CacheTestBase {
     }
     servers_.reset(new AprMemCache(server_spec_, 5, hasher, &statistics_,
                                    &timer_, &handler_));
-    cache_.reset(new FallbackCache(servers_.get(), lru_cache_.get(),
+    // As memcached is not restarted between tests, we need some other kind of
+    // isolation. One option would be to flush memcached, if apr_memcache
+    // supported that. We do not want to modify our fork even further, so we
+    // prepend the test name and current time to all keys that go to memcached.
+    const ::testing::TestInfo* const test_info =
+        ::testing::UnitTest::GetInstance()->current_test_info();
+    PosixTimer timer;
+    const GoogleString memcache_prefix =
+        StrCat(test_info->test_case_name(), ".",
+               test_info->name(), "_",
+               Integer64ToString(timer.NowUs()), "_");
+    prefixed_memcache_.reset(
+        new CacheKeyPrepender(memcache_prefix, servers_.get()));
+
+    cache_.reset(new FallbackCache(prefixed_memcache_.get(), lru_cache_.get(),
                                    kTestValueSizeThreshold,
                                    &handler_));
 
@@ -137,6 +153,7 @@ class AprMemCacheTest : public CacheTestBase {
   MockTimer timer_;
   scoped_ptr<LRUCache> lru_cache_;
   scoped_ptr<AprMemCache> servers_;
+  scoped_ptr<CacheKeyPrepender> prefixed_memcache_;
   scoped_ptr<FallbackCache> cache_;
   scoped_ptr<ThreadSystem> thread_system_;
   SimpleStats statistics_;
@@ -322,7 +339,7 @@ TEST_F(AprMemCacheTest, MultiServerFallback) {
   // Make another connection to the same memcached, but with a different
   // fallback cache.
   LRUCache lru_cache2(kLRUCacheSize);
-  FallbackCache mem_cache2(servers_.get(), &lru_cache2,
+  FallbackCache mem_cache2(prefixed_memcache_.get(), &lru_cache2,
                            kTestValueSizeThreshold,
                            &handler_);
 
@@ -356,7 +373,7 @@ TEST_F(AprMemCacheTest, KeyOver64kDropped) {
   const int kBigLruSize = 1000000;
   const int kThreshold =  200000;    // fits key and small value.
   LRUCache lru_cache2(kLRUCacheSize);
-  FallbackCache mem_cache2(servers_.get(), &lru_cache2,
+  FallbackCache mem_cache2(prefixed_memcache_.get(), &lru_cache2,
                            kThreshold, &handler_);
 
   const GoogleString kKey(kBigLruSize, 'a');
@@ -412,7 +429,7 @@ TEST_F(AprMemCacheTest, ThreadSafe) {
                          200 /* num_iters */,
                          10 /* num_inserts */,
                          false, true, large_pattern.c_str(),
-                         servers_.get(),
+                         prefixed_memcache_.get(),
                          thread_system_.get());
 }
 
@@ -489,4 +506,23 @@ TEST_F(AprMemCacheTest, OneMicrosecondDelete) {
   EXPECT_EQ(1, statistics_.GetVariable("memcache_timeouts")->Get());
 }
 
+// Two following tests are identical and ensure that no keys are leaked between
+// tests through shared running Memcached server.
+TEST_F(AprMemCacheTest, TestsAreIsolated1) {
+  if (!InitMemcachedOrSkip(true)) {
+    return;
+  }
+
+  CheckNotFound("SomeKey");
+  CheckPut("SomeKey", "SomeValue");
+}
+
+TEST_F(AprMemCacheTest, TestsAreIsolated2) {
+  if (!InitMemcachedOrSkip(true)) {
+    return;
+  }
+
+  CheckNotFound("SomeKey");
+  CheckPut("SomeKey", "SomeValue");
+}
 }  // namespace net_instaweb

pagespeed/system/redis_cache.cc

@@ -120,6 +120,15 @@ void RedisCache::Delete(const GoogleString& key) {
   ValidateRedisReply(reply, {REDIS_REPLY_INTEGER}, "DEL");
 }
 
+bool RedisCache::FlushAll() {
+  if (!IsHealthy()) {
+    return false;
+  }
+
+  RedisReply reply = redisCommand("FLUSHALL");
+  return ValidateRedisReply(reply, {REDIS_REPLY_STATUS}, "FLUSHALL");
+}
+
 RedisCache::RedisReply RedisCache::redisCommand(const char* format, ...) {
   CHECK(redis_ != nullptr);
   va_list args;

pagespeed/system/redis_cache.h

@@ -61,6 +61,9 @@ class RedisCache : public CacheInterface {
   bool IsHealthy() const override;
   void ShutDown() override;
 
+  // Flushes ALL DATA IN REDIS in blocking mode. Used in tests
+  bool FlushAll();
+
  private:
   GoogleString host_;
   int port_;

pagespeed/system/redis_cache_test.cc

@@ -30,6 +30,8 @@
 
 namespace net_instaweb {
 
+// TODO(yeputons): refactor this class with AprMemCacheTest, see details in
+// apr_mem_cache_test.cc
 class RedisCacheTest : public CacheTestBase {
  protected:
   RedisCacheTest() {}
@@ -46,13 +48,13 @@ class RedisCacheTest : public CacheTestBase {
     }
 
     cache_.reset(new RedisCache("localhost", port, &handler_));
-    return cache_->Connect();
+    return cache_->Connect() && cache_->FlushAll();
   }
 
   CacheInterface* Cache() override { return cache_.get(); }
 
- private:
   scoped_ptr<RedisCache> cache_;
+ private:
   GoogleMessageHandler handler_;
 };
 
@@ -94,4 +96,37 @@ TEST_F(RedisCacheTest, BasicInvalid) {
   CheckGet("nameB", "valueB");
 }
 
+
+TEST_F(RedisCacheTest, FlushAll) {
+  if (!InitRedisOrSkip()) {
+    return;
+  }
+
+  CheckPut("Name1", "Value1");
+  CheckPut("Name2", "Value2");
+  cache_->FlushAll();
+  CheckNotFound("Name1");
+  CheckNotFound("Name2");
+}
+
+// Two following tests are identical and ensure that no keys are leaked between
+// tests through shared running Redis server.
+TEST_F(RedisCacheTest, TestsAreIsolated1) {
+  if (!InitRedisOrSkip()) {
+    return;
+  }
+
+  CheckNotFound("SomeKey");
+  CheckPut("SomeKey", "SomeValue");
+}
+
+TEST_F(RedisCacheTest, TestsAreIsolated2) {
+  if (!InitRedisOrSkip()) {
+    return;
+  }
+
+  CheckNotFound("SomeKey");
+  CheckPut("SomeKey", "SomeValue");
+}
+
 }  // namespace net_instaweb