On config change, only clear connection managers for changed config (#…

…971)

Follows up #968.

As a relic from when we had global configuration, anytime any config
value is changed on any client, we still clear all connection managers
everywhere on every thread, even though this is not necessary. This
means that we lose all open connections, etc.

Here, we make changes so that if a configuration is changed, we only
clear the configuration managers pertaining to that one particular
configuration, thus conserving resources globally.

Co-authored-by: Brandur <brandur@brandur.org>

lib/stripe/stripe_client.rb

@@ -14,28 +14,29 @@ class StripeClient
     @last_connection_manager_gc = Util.monotonic_time
 
     # Initializes a new StripeClient
-    def initialize(config_overrides = {})
+    def initialize(config_arg = {})
       @system_profiler = SystemProfiler.new
       @last_request_metrics = nil
 
-      # Supports accepting a connection manager object for backwards
-      # compatibility only, and that use is DEPRECATED.
-      @config_overrides = case config_overrides
-                          when Stripe::ConnectionManager
-                            {}
-                          when String
-                            { api_key: config_overrides }
-                          else
-                            config_overrides
-                          end
-    end
-
-    # Always base config off the global Stripe configuration to ensure the
-    # client picks up any changes to the config.
-    def config
-      Stripe.configuration.reverse_duplicate_merge(@config_overrides)
-    end
-
+      @config = case config_arg
+                when Hash
+                  Stripe.configuration.reverse_duplicate_merge(config_arg)
+                when Stripe::ConnectionManager
+                  # Supports accepting a connection manager object for backwards
+                  # compatibility only, and that use is DEPRECATED.
+                  Stripe.configuration.dup
+                when Stripe::StripeConfiguration
+                  config_arg
+                when String
+                  Stripe.configuration.reverse_duplicate_merge(
+                    { api_key: config_arg }
+                  )
+                else
+                  raise ArgumentError, "Can't handle argument: #{config_arg}"
+                end
+    end
+
+    attr_reader :config
     attr_reader :options
 
     # Gets a currently active `StripeClient`. Set for the current thread when
@@ -53,30 +54,45 @@ def self.active_client
     # clears them from internal tracking in all connection managers across all
     # threads.
     #
+    # If passed a `config` object, only clear connection managers for that
+    # particular configuration.
+    #
     # For internal use only. Does not provide a stable API and may be broken
     # with future non-major changes.
-    def self.clear_all_connection_managers
+    def self.clear_all_connection_managers(config: nil)
       # Just a quick path for when configuration is being set for the first
       # time before any connections have been opened. There is technically some
       # potential for thread raciness here, but not in a practical sense.
       return if @thread_contexts_with_connection_managers.empty?
 
       @thread_contexts_with_connection_managers_mutex.synchronize do
+        pruned_contexts = Set.new
+
         @thread_contexts_with_connection_managers.each do |thread_context|
           # Note that the thread context itself is not destroyed, but we clear
           # its connection manager and remove our reference to it. If it ever
           # makes a new request we'll give it a new connection manager and
           # it'll go back into `@thread_contexts_with_connection_managers`.
-          thread_context.default_connection_managers.map { |_, cm| cm.clear }
-          thread_context.reset_connection_managers
+          thread_context.default_connection_managers.reject! do |cm_config, cm|
+            if config.nil? || config.key == cm_config
+              cm.clear
+              true
+            end
+          end
+
+          if thread_context.default_connection_managers.empty?
+            pruned_contexts << thread_context
+          end
         end
-        @thread_contexts_with_connection_managers.clear
+
+        @thread_contexts_with_connection_managers.subtract(pruned_contexts)
       end
     end
 
     # A default client for the current thread.
     def self.default_client
-      current_thread_context.default_client ||= StripeClient.new
+      current_thread_context.default_client ||=
+        StripeClient.new(Stripe.configuration)
     end
 
     # A default connection manager for the current thread scoped to the
@@ -412,7 +428,7 @@ def self.maybe_gc_connection_managers
       last_used_threshold =
         Util.monotonic_time - CONNECTION_MANAGER_GC_LAST_USED_EXPIRY
 
-      pruned_thread_contexts = []
+      pruned_contexts = []
       @thread_contexts_with_connection_managers.each do |thread_context|
         thread_context
           .default_connection_managers
@@ -427,13 +443,13 @@ def self.maybe_gc_connection_managers
       @thread_contexts_with_connection_managers.each do |thread_context|
         next unless thread_context.default_connection_managers.empty?
 
-        pruned_thread_contexts << thread_context
+        pruned_contexts << thread_context
       end
 
-      @thread_contexts_with_connection_managers -= pruned_thread_contexts
+      @thread_contexts_with_connection_managers -= pruned_contexts
       @last_connection_manager_gc = Util.monotonic_time
 
-      pruned_thread_contexts.count
+      pruned_contexts.count
     end
 
     private def api_url(url = "", api_base = nil)

lib/stripe/stripe_configuration.rb

@@ -111,12 +111,12 @@ def initial_network_retry_delay=(val)
 
     def open_timeout=(open_timeout)
       @open_timeout = open_timeout
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def read_timeout=(read_timeout)
       @read_timeout = read_timeout
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def write_timeout=(write_timeout)
@@ -125,32 +125,32 @@ def write_timeout=(write_timeout)
       end
 
       @write_timeout = write_timeout
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def proxy=(proxy)
       @proxy = proxy
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def verify_ssl_certs=(verify_ssl_certs)
       @verify_ssl_certs = verify_ssl_certs
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def uploads_base=(uploads_base)
       @uploads_base = uploads_base
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def connect_base=(connect_base)
       @connect_base = connect_base
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def api_base=(api_base)
       @api_base = api_base
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     def ca_bundle_path=(path)
@@ -159,7 +159,7 @@ def ca_bundle_path=(path)
       # empty this field so a new store is initialized
       @ca_store = nil
 
-      StripeClient.clear_all_connection_managers
+      StripeClient.clear_all_connection_managers(config: self)
     end
 
     # A certificate store initialized from the the bundle in #ca_bundle_path and

test/stripe/stripe_client_test.rb

@@ -20,6 +20,12 @@ class StripeClientTest < Test::Unit::TestCase
       should "support deprecated ConnectionManager objects" do
         assert StripeClient.new(Stripe::ConnectionManager.new).config.is_a?(Stripe::StripeConfiguration)
       end
+
+      should "support passing a full configuration object" do
+        config = Stripe.configuration.reverse_duplicate_merge({ api_key: "test_123", open_timeout: 100 })
+        client = StripeClient.new(config)
+        assert_equal config, client.config
+      end
     end
 
     context ".active_client" do
@@ -203,6 +209,27 @@ class StripeClientTest < Test::Unit::TestCase
         # And finally, give all threads time to perform their check.
         threads.each(&:join)
       end
+
+      should "clear only connection managers with a given configuration" do
+        StripeClient.clear_all_connection_managers
+
+        client1 = StripeClient.new(read_timeout: 5.0)
+        StripeClient.default_connection_manager(client1.config)
+        client2 = StripeClient.new(read_timeout: 2.0)
+        StripeClient.default_connection_manager(client2.config)
+
+        thread_contexts = StripeClient.instance_variable_get(:@thread_contexts_with_connection_managers)
+        assert_equal 1, thread_contexts.count
+        thread_context = thread_contexts.first
+
+        refute_nil thread_context.default_connection_managers[client1.config.key]
+        refute_nil thread_context.default_connection_managers[client2.config.key]
+
+        StripeClient.clear_all_connection_managers(config: client1.config)
+
+        assert_nil thread_context.default_connection_managers[client1.config.key]
+        refute_nil thread_context.default_connection_managers[client2.config.key]
+      end
     end
 
     context ".default_client" do
@@ -244,7 +271,9 @@ class StripeClientTest < Test::Unit::TestCase
         assert_equal connection_manager_two.config.open_timeout, 100
       end
 
-      should "create a single connection manager for identitical configurations" do
+      should "create a single connection manager for identical configurations" do
+        StripeClient.clear_all_connection_managers
+
         2.times { StripeClient.default_connection_manager(Stripe::StripeConfiguration.setup) }
 
         assert_equal 1, StripeClient.instance_variable_get(:@thread_contexts_with_connection_managers).first.default_connection_managers.size

test/stripe/stripe_configuration_test.rb

@@ -100,49 +100,49 @@ class StripeConfigurationTest < Test::Unit::TestCase
       should "clear when setting allow ca_bundle_path" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.ca_bundle_path = "/path/to/ca/bundle"
       end
 
       should "clear when setting open timeout" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.open_timeout = 10
       end
 
       should "clear when setting read timeout" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.read_timeout = 10
       end
 
       should "clear when setting uploads_base" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.uploads_base = "https://other.stripe.com"
       end
 
-      should "clearn when setting api_base to be configured" do
+      should "clear when setting api_base to be configured" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.api_base = "https://other.stripe.com"
       end
 
       should "clear when setting connect_base" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.connect_base = "https://other.stripe.com"
       end
 
       should "clear when setting verify_ssl_certs" do
         config = Stripe::StripeConfiguration.setup
 
-        StripeClient.expects(:clear_all_connection_managers)
+        StripeClient.expects(:clear_all_connection_managers).with(config: config)
         config.verify_ssl_certs = false
       end
     end