querier_cache: simplify memory eviction use-after-free fix, add tests

Simplify the fix for memory based eviction, introduced by 918d255 so
there is no need to massage the counters.

Also add a check to `test_memory_based_cache_eviction` which checks for
the bug fixed. While at it also add a check to
`test_time_based_cache_eviction` for the fix to time based eviction
(e5a0ea3).

Tests: tests/querier_cache:debug
Signed-off-by: Botond Dénes <bdenes@scylladb.com>
Message-Id: <c89e2788a88c2a701a2c39f377328e77ac01e3ef.1546515465.git.bdenes@scylladb.com>

querier.cc

@@ -274,14 +274,12 @@ static void insert_querier(
     memory_usage += q.memory_usage();
 
     if (memory_usage >= max_queriers_memory_usage) {
-        while (!entries.empty() && memory_usage >= max_queriers_memory_usage) {
-            auto it = entries.begin();
+        auto it = entries.begin();
+        while (it != entries.end() && memory_usage >= max_queriers_memory_usage) {
             memory_usage -= it->memory_usage();
-            auto ir = sem.unregister_inactive_read(it->get_inactive_handle());
-            ir->evict();
-            // querier_inactive_read::evict() updates resource_based_evictions,
-            // (and population) while we want memory_based_evictions:
-            --stats.resource_based_evictions;
+            sem.unregister_inactive_read(it->get_inactive_handle());
+            it = entries.erase(it);
+            --stats.population;
             ++stats.memory_based_evictions;
         }
     }

reader_concurrency_semaphore.hh

@@ -228,6 +228,10 @@ public:
     /// (if there was no reader to evict).
     bool try_evict_one_inactive_read();
 
+    size_t inactive_reads() const {
+        return _inactive_reads.size();
+    }
+
     void clear_inactive_reads() {
         _inactive_reads.clear();
     }

tests/querier_cache.cc

@@ -564,6 +564,10 @@ SEASTAR_THREAD_TEST_CASE(test_time_based_cache_eviction) {
         .misses()
         .no_drops()
         .time_based_evictions();
+
+    // There should be no inactive reads, the querier_cache should unregister
+    // the expired queriers.
+    BOOST_REQUIRE_EQUAL(t.get_semaphore().inactive_reads(), 0);
 }
 
 sstring make_string_blob(size_t size) {
@@ -598,6 +602,8 @@ SEASTAR_THREAD_TEST_CASE(test_memory_based_cache_eviction) {
         t.produce_first_page_and_save_data_querier(i);
     }
 
+    const auto pop_before = t.get_semaphore().inactive_reads();
+
     // Should overflow the limit and trigger the eviction of the oldest entry.
     t.produce_first_page_and_save_data_querier(queriers_needed_to_fill_cache);
 
@@ -605,6 +611,10 @@ SEASTAR_THREAD_TEST_CASE(test_memory_based_cache_eviction) {
         .misses()
         .no_drops()
         .memory_based_evictions();
+
+    // Since the last insert should have evicted an existing entry, we should
+    // have the same number of registered inactive reads.
+    BOOST_REQUIRE_EQUAL(t.get_semaphore().inactive_reads(), pop_before);
 }
 
 SEASTAR_THREAD_TEST_CASE(test_resources_based_cache_eviction) {