BackgroundHelper: avoid leaking threads (#475)

* Fix activerecord trilogy adapter config override test

* BackgroundHelper: avoid leaking threads

Many test cases would define `teardown` without calling super
so we were never killing threads...

It's better to use `before_teardown` anyway, and it's less popular
so less likely to be overriden without a super.

This is very likely to be the cause of the crash we've seen
at https://github.com/Shopify/semian/actions/runs/4122969863/jobs/7120418231

Co-Authored-By: Adrianna Chang <adrianna.chang@shopify.com>

* Rescue timeout errors from background thread

---------

Co-authored-by: Adrianna Chang <adrianna.chang@shopify.com>
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>

test/adapters/activerecord_trilogy_adapter_test.rb

@@ -6,6 +6,8 @@
 module ActiveRecord
   module ConnectionAdapters
     class ActiveRecordTrilogyAdapterTest < Minitest::Test
+      include BackgroundHelper
+
       ERROR_TIMEOUT = 5
       ERROR_THRESHOLD = 1
       SEMIAN_OPTIONS = {
@@ -18,6 +20,7 @@ class ActiveRecordTrilogyAdapterTest < Minitest::Test
       }
 
       def setup
+        super
         @proxy = Toxiproxy[:semian_test_mysql]
         Semian.destroy(:mysql_testing)
 
@@ -34,6 +37,7 @@ def setup
       end
 
       def teardown
+        super
         @adapter.disconnect!
       end
 
@@ -60,12 +64,10 @@ def test_semian_can_be_disabled
       end
 
       def test_adapter_does_not_modify_config
-        config = @configuration.merge(config_overrides)
-
-        assert(config.key?(:semian))
-        TrilogyAdapter.new(config)
+        assert(@configuration.key?(:semian))
+        TrilogyAdapter.new(@configuration)
 
-        assert(config.key?(:semian))
+        assert(@configuration.key?(:semian))
       end
 
       def test_unconfigured

test/adapters/mysql2_test.rb

@@ -5,6 +5,8 @@
 require "semian/mysql2"
 
 class TestMysql2 < Minitest::Test
+  include BackgroundHelper
+
   ERROR_TIMEOUT = 5
   ERROR_THRESHOLD = 1
   SEMIAN_OPTIONS = {
@@ -17,6 +19,7 @@ class TestMysql2 < Minitest::Test
   }
 
   def setup
+    super
     @proxy = Toxiproxy[:semian_test_mysql]
     Semian.destroy(:mysql_testing)
   end

test/adapters/redis_client_test.rb

@@ -7,6 +7,8 @@
 require "benchmark"
 
 module RedisClientTests
+  include BackgroundHelper
+
   REDIS_TIMEOUT = 0.5
   ERROR_TIMEOUT = 5
   ERROR_THRESHOLD = 1
@@ -20,9 +22,8 @@ module RedisClientTests
     error_timeout: ERROR_TIMEOUT,
   }
 
-  attr_writer :threads
-
   def setup
+    super
     @proxy = Toxiproxy[:semian_test_redis]
     Semian.destroy(:redis_testing)
   end
@@ -156,7 +157,11 @@ def test_other_redis_errors_are_not_tagged_with_the_resource_identifier
 
   def test_resource_timeout_on_connect
     @proxy.downstream(:latency, latency: redis_timeout_ms).apply do
-      background { connect_to_redis! }
+      background do
+        connect_to_redis!
+      rescue RedisClient::CircuitOpenError
+        nil
+      end
 
       assert_raises(RedisClient::ResourceBusyError) do
         connect_to_redis!

test/adapters/redis_test.rb

@@ -8,6 +8,8 @@
 require "semian/redis"
 
 module RedisTests
+  include BackgroundHelper
+
   REDIS_TIMEOUT = 0.5
   ERROR_TIMEOUT = 5
   ERROR_THRESHOLD = 1
@@ -21,9 +23,8 @@ module RedisTests
     error_timeout: ERROR_TIMEOUT,
   }
 
-  attr_writer :threads
-
   def setup
+    super
     @proxy = Toxiproxy[:semian_test_redis]
     Semian.destroy(:redis_testing)
   end
@@ -198,7 +199,11 @@ def test_other_redis_errors_are_not_tagged_with_the_resource_identifier
 
   def test_resource_timeout_on_connect
     @proxy.downstream(:latency, latency: redis_timeout_ms).apply do
-      background { connect_to_redis! }
+      background do
+        connect_to_redis!
+      rescue Redis::CircuitOpenError
+        nil
+      end
 
       assert_raises(Redis::ResourceBusyError) do
         connect_to_redis!

test/helpers/background_helper.rb

@@ -1,28 +1,27 @@
 # frozen_string_literal: true
 
 module BackgroundHelper
-  attr_writer :threads
+  def after_setup
+    @_threads = []
+  end
 
-  def teardown
-    threads.each(&:kill)
-    self.threads = []
+  def before_teardown
+    @_threads.each(&:kill)
+    @_threads.each(&:join)
+    @_threads = nil
   end
 
   private
 
   def background(&block)
     thread = Thread.new(&block)
     thread.report_on_exception = false
-    threads << thread
+    @_threads << thread
     thread.join(0.1)
     thread
   end
 
-  def threads
-    @threads ||= []
-  end
-
   def yield_to_background
-    threads.each(&:join)
+    @_threads.each(&:join)
   end
 end

test/protected_resource_test.rb

@@ -6,6 +6,7 @@
 class TestProtectedResource < Minitest::Test
   include CircuitBreakerHelper
   include ResourceHelper
+  include BackgroundHelper
 
   def setup
     destroy_all_semian_resources

test/test_helper.rb

@@ -64,7 +64,6 @@
 
 module Minitest
   class Test
-    include BackgroundHelper
     include TimeHelper
     include ResourceHelper
   end