Replace Time.now with monotonic in lru cache and resource update

CHANGELOG.md

@@ -7,6 +7,8 @@
   variable `SEMIAN_DISABLED` is set. (#427)
   Introduces environment variables `SEMIAN_CIRCUIT_BREAKER_DISABLED` to disable only
   `circuit_breaker` and `SEMIAN_BULKHEAD_DISABLED` only `bulkhead`.
+* Refactor: Replace `Time.now` with `CLOCK_MONOTONIC` in Resource `updated_at` field. (#443)
+  Replace `Timecop.travel` for tests with custom mock solution to support monotonic clocks.
 
 # v0.16.0
 

README.md

@@ -111,7 +111,7 @@ There are some global configuration options that can be set for Semian:
 # Note: Setting this to 0 enables aggressive garbage collection.
 Semian.maximum_lru_size = 0
 
-# Minimum time a resource should be resident in the LRU cache (default: 300s)
+# Minimum time in seconds a resource should be resident in the LRU cache (default: 300s)
 Semian.minimum_lru_time = 60
 ```
 

lib/semian.rb

@@ -104,7 +104,7 @@ module Semian
   attr_accessor :maximum_lru_size, :minimum_lru_time, :default_permissions, :namespace
 
   self.maximum_lru_size = 500
-  self.minimum_lru_time = 300
+  self.minimum_lru_time = 300 # 300 seconds / 5 minutes
   self.default_permissions = 0660
 
   def issue_disabled_semaphores_warning

lib/semian/lru_hash.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "thread"
+
 class LRUHash
   # This LRU (Least Recently Used) hash will allow
   # the cleaning of resources as time goes on.
@@ -41,7 +43,7 @@ def clear
   #
   # Arguments:
   #   +max_size+ The maximum size of the table
-  #   +min_time+ The minimum time a resource can live in the cache
+  #   +min_time+ The minimum time in seconds a resource can live in the cache
   #
   # Note:
   #   The +min_time+ is a stronger guarantee than +max_size+. That is, if there are
@@ -57,9 +59,9 @@ def initialize(max_size: Semian.maximum_lru_size, min_time: Semian.minimum_lru_t
     @table = {}
     @lock =
       if Semian.thread_safe?
-        Mutex.new
+        ::Thread::Mutex.new
       else
-        NoopMutex.new
+        ::LRUHash::NoopMutex.new
       end
   end
 
@@ -83,7 +85,7 @@ def set(key, resource)
     @lock.synchronize do
       @table.delete(key)
       @table[key] = resource
-      resource.updated_at = Time.now
+      resource.updated_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
     end
     clear_unused_resources if @table.length > @max_size
   end
@@ -98,7 +100,7 @@ def get(key)
       found = @table.delete(key)
       if found
         @table[key] = found
-        found.updated_at = Time.now
+        found.updated_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
       end
       found
     end
@@ -130,9 +132,10 @@ def clear_unused_resources
     timer_start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
 
     ran = try_synchronize do
-      # Clears resources that have not been used in the last 5 minutes.
+      # Clears resources that have not been used
+      # in the last 5 minutes (default value of Semian.minimum_lru_time).
 
-      stop_time = Time.now - @min_time # Don't process resources updated after this time
+      stop_time = Process.clock_gettime(Process::CLOCK_MONOTONIC) - @min_time
       @table.each do |_, resource|
         payload[:examined] += 1
 

lib/semian/protected_resource.rb

@@ -15,7 +15,7 @@ def initialize(name, bulkhead, circuit_breaker)
       @name = name
       @bulkhead = bulkhead
       @circuit_breaker = circuit_breaker
-      @updated_at = Time.now
+      @updated_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
     end
 
     def destroy

lib/semian/unprotected_resource.rb

@@ -9,7 +9,7 @@ class UnprotectedResource
 
     def initialize(name)
       @name = name
-      @updated_at = Time.now
+      @updated_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
     end
 
     def registered_workers

test/helpers/time_helper.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module TimeHelper
+  def time_travel(val)
+    now_monotonic = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+    now_timestamp = Time.now
+
+    new_monotonic = now_monotonic + val
+    Process.stubs(:clock_gettime).with(Process::CLOCK_MONOTONIC).returns(new_monotonic)
+
+    new_timestamp = now_timestamp + val
+    Time.stubs(:now).returns(new_timestamp)
+
+    yield
+  ensure
+    Time.unstub(:now)
+    Process.unstub(:clock_gettime)
+  end
+end

test/lru_hash_test.rb

@@ -3,6 +3,8 @@
 require "test_helper"
 
 class TestLRUHash < Minitest::Test
+  include TimeHelper
+
   def setup
     Semian.thread_safe = true
     @lru_hash = LRUHash.new(max_size: 0)
@@ -58,35 +60,39 @@ def test_get_moves_the_item_at_the_top
   end
 
   def test_set_cleans_resources_if_last_error_has_expired
-    @lru_hash.set("b", create_circuit_breaker("b", true, false, 1000))
+    lru_hash = LRUHash.new(max_size: 0, min_time: 1)
+    lru_hash.set("b", create_circuit_breaker("b", true, false, 2))
 
-    Timecop.travel(2000) do
-      @lru_hash.set("d", create_circuit_breaker("d"))
+    time_travel(4) do
+      lru_hash.set("d", create_circuit_breaker("d"))
 
-      assert_equal(1, @lru_hash.size)
+      assert_equal(["d"], lru_hash.values.map(&:name))
+      assert_equal(1, lru_hash.size)
     end
   end
 
   def test_set_does_not_clean_resources_if_last_error_has_not_expired
     @lru_hash.set("b", create_circuit_breaker("b", true, false, 1000))
 
-    Timecop.travel(600) do
+    time_travel(600) do
       @lru_hash.set("d", create_circuit_breaker("d"))
-
-      assert_equal(2, @lru_hash.size)
     end
+
+    assert_equal(2, @lru_hash.size)
   end
 
   def test_set_cleans_resources_if_minimum_time_is_reached
-    @lru_hash.set("a", create_circuit_breaker("a", true, false, 1000))
-    @lru_hash.set("b", create_circuit_breaker("b", false))
-    @lru_hash.set("c", create_circuit_breaker("c", false))
-
-    Timecop.travel(600) do
-      @lru_hash.set("d", create_circuit_breaker("d"))
+    lru_hash = LRUHash.new(max_size: 0, min_time: 1)
+    lru_hash.set("a", create_circuit_breaker("a", true, false, 1000))
+    lru_hash.set("b", create_circuit_breaker("b", false))
+    lru_hash.set("c", create_circuit_breaker("c", false))
 
-      assert_equal(2, @lru_hash.size)
+    time_travel(2) do
+      lru_hash.set("d", create_circuit_breaker("d"))
     end
+
+    assert_equal(["a", "d"], lru_hash.values.map(&:name))
+    assert_equal(2, lru_hash.size)
   end
 
   def test_set_does_not_cleans_resources_if_minimum_time_is_not_reached
@@ -121,17 +127,18 @@ def test_clear
   end
 
   def test_clean_instrumentation
-    @lru_hash.set("a", create_circuit_breaker("a", true, false, 1000))
-    @lru_hash.set("b", create_circuit_breaker("b", true, false, 1000))
-    @lru_hash.set("c", create_circuit_breaker("c", true, false, 1000))
+    lru_hash = LRUHash.new(max_size: 0, min_time: 1)
+    lru_hash.set("a", create_circuit_breaker("a", true, false, 2))
+    lru_hash.set("b", create_circuit_breaker("b", true, false, 2))
+    lru_hash.set("c", create_circuit_breaker("c", true, false, 2))
 
     notified = false
     subscriber = Semian.subscribe do |event, resource, scope, adapter, payload|
       next unless event == :lru_hash_gc
 
       notified = true
 
-      assert_equal(@lru_hash, resource)
+      assert_equal(lru_hash, resource)
       assert_nil(scope)
       assert_nil(adapter)
       assert_equal(4, payload[:size])
@@ -140,8 +147,8 @@ def test_clean_instrumentation
       refute_nil(payload[:elapsed])
     end
 
-    Timecop.travel(2000) do
-      @lru_hash.set("d", create_circuit_breaker("d"))
+    time_travel(3) do
+      lru_hash.set("d", create_circuit_breaker("d"))
     end
 
     assert(notified, "No notifications has been emitted")
@@ -150,7 +157,8 @@ def test_clean_instrumentation
   end
 
   def test_monotonically_increasing
-    start_time = Time.now
+    start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+    lru_hash = LRUHash.new(max_size: 0, min_time: 3)
 
     notification = 0
     subscriber = Semian.subscribe do |event, _resource, _scope, _adapter, payload|
@@ -169,79 +177,81 @@ def test_monotonically_increasing
       end
     end
 
-    assert_monotonic = lambda do
+    assert_monotonic = lambda do |lru_hash|
       previous_timestamp = start_time
 
-      @lru_hash.keys.zip(@lru_hash.values).each do |key, val|
+      lru_hash.keys.zip(lru_hash.values).each do |key, val|
         assert(val.updated_at > previous_timestamp, "Timestamp for #{key} was not monotonically increasing")
       end
     end
 
-    @lru_hash.set("a", create_circuit_breaker("a"))
-    @lru_hash.set("b", create_circuit_breaker("b"))
-    @lru_hash.set("c", create_circuit_breaker("c"))
+    lru_hash.set("a", create_circuit_breaker("a"))
+    lru_hash.set("b", create_circuit_breaker("b"))
+    lru_hash.set("c", create_circuit_breaker("c"))
 
-    # Before: [a, b, c]
-    # After: [a, c, b]
-    Timecop.travel(Semian.minimum_lru_time - 1) do
-      @lru_hash.get("b")
-      assert_monotonic.call
-    end
+    assert_equal(["a", "b", "c"], lru_hash.values.map(&:name))
 
-    # Before: [a, c, b]
-    # After: [a, c, b, d]
-    Timecop.travel(Semian.minimum_lru_time - 1) do
-      @lru_hash.set("d", create_circuit_breaker("d"))
-      assert_monotonic.call
+    time_travel(1) do
+      lru_hash.get("b")
+      assert_monotonic.call(lru_hash)
+
+      assert_equal(["a", "c", "b"], lru_hash.values.map(&:name))
+
+      lru_hash.set("d", create_circuit_breaker("d"))
     end
+    assert_monotonic.call(lru_hash)
 
-    # Before: [a, c, b, d]
-    # After: [b, d, e]
-    Timecop.travel(Semian.minimum_lru_time) do
-      @lru_hash.set("e", create_circuit_breaker("e"))
-      assert_monotonic.call
+    assert_equal(["a", "c", "b", "d"], lru_hash.values.map(&:name))
+
+    time_travel(3) do
+      lru_hash.set("e", create_circuit_breaker("e"))
     end
+    assert_monotonic.call(lru_hash)
+
+    assert_equal(["b", "d", "e"], lru_hash.values.map(&:name))
   ensure
     Semian.unsubscribe(subscriber)
   end
 
   def test_max_size
-    lru_hash = LRUHash.new(max_size: 3)
+    lru_hash = LRUHash.new(max_size: 3, min_time: 1)
     lru_hash.set("a", create_circuit_breaker("a"))
     lru_hash.set("b", create_circuit_breaker("b"))
     lru_hash.set("c", create_circuit_breaker("c"))
 
     assert_equal(3, lru_hash.size)
+    assert_equal(["a", "b", "c"], lru_hash.values.map(&:name))
 
-    Timecop.travel(Semian.minimum_lru_time) do
-      # [a, b, c] are older than the min_time, so they get garbage collected.
-      lru_hash.set("d", create_circuit_breaker("d"))
+    sleep(1)
+    lru_hash.set("d", create_circuit_breaker("d"))
 
-      assert_equal(1, lru_hash.size)
-    end
+    assert_equal(["d"], lru_hash.values.map(&:name))
   end
 
   def test_max_size_overflow
-    lru_hash = LRUHash.new(max_size: 3)
+    lru_hash = LRUHash.new(max_size: 3, min_time: 3)
     lru_hash.set("a", create_circuit_breaker("a"))
     lru_hash.set("b", create_circuit_breaker("b"))
 
     assert_equal(2, lru_hash.size)
+    assert_equal(["a", "b"], lru_hash.values.map(&:name))
 
-    Timecop.travel(Semian.minimum_lru_time) do
+    time_travel(5) do
       # [a, b] are older than the min_time, but the hash isn't full, so
       # there's no garbage collection.
       lru_hash.set("c", create_circuit_breaker("c"))
-
-      assert_equal(3, lru_hash.size)
     end
 
-    Timecop.travel(Semian.minimum_lru_time + 1) do
+    assert_equal(3, lru_hash.size)
+    assert_equal(["a", "b", "c"], lru_hash.values.map(&:name))
+
+    time_travel(6) do
       # [a, b] are beyond the min_time, but [c] isn't.
       lru_hash.set("d", create_circuit_breaker("d"))
-
-      assert_equal(2, lru_hash.size)
     end
+
+    assert_equal(2, lru_hash.size)
+    assert_equal(["c", "d"], lru_hash.values.map(&:name))
   end
 
   private

test/test_helper.rb

@@ -12,11 +12,12 @@
 require "mocha"
 require "mocha/minitest"
 
+require "helpers/adapter_helper"
 require "helpers/background_helper"
 require "helpers/circuit_breaker_helper"
-require "helpers/resource_helper"
-require "helpers/adapter_helper"
 require "helpers/mock_server.rb"
+require "helpers/resource_helper"
+require "helpers/time_helper.rb"
 
 require "config/semian_config"