On config change, only clear connection managers for changed config #971
Conversation
| @@ -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 = {}) | |||
There was a problem hiding this comment.
Still supports the same types (i.e. hash, string, etc.) before, but I've expanded it so that you can optionally inject a full configuration object. The reason is that it allows me to initialize all default clients with the same global configuration object (Stripe.configuration). That means that when global configuration changes (i.e. Stripe.write_timeout = 3.0), that new configuration propagates to every default client.
There was a problem hiding this comment.
This is fantastic and feels much more consistent! When I was writing specs, I found that I wanted to pass a StripeConfiguration object but had to remind myself to use a Hash.
There was a problem hiding this comment.
Perfect. Thanks!
| Stripe.configuration.reverse_duplicate_merge(@config_overrides) | ||
| end | ||
|
|
||
| @config = case config_arg |
There was a problem hiding this comment.
See comment above, but eliminated the "lazy" configuration initialization with always setting @config directly on initialization.
| @@ -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 = [] | |||
There was a problem hiding this comment.
Ran into lint line length problems on the code above so I gave the equivalent variable a shorter name. I renamed this one too for consistency.
| @@ -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 | |||
There was a problem hiding this comment.
Because connection managers are now cleared more granularly, it's possible to have some leftover from previous tests. Force clear them here so that we get the expected numbers below.
There was a problem hiding this comment.
Oh, my. I routinely put my English degree to shame. Sorry about that typo 😄
There was a problem hiding this comment.
Haha, you'd need a pretty advanced degree to stop making typos ;)
brandur-stripe
force-pushed
the
brandur-only-clear-connection-managers-for-changed-config
branch
from
6e0e440 to
b0a6629
Compare
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.
brandur-stripe
force-pushed
the
brandur-only-clear-connection-managers-for-changed-config
branch
from
b0a6629 to
1e1f602
Compare
|
Thank you Joel! r? @richardm-stripe Mind also taking a look? |
| # 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) |
There was a problem hiding this comment.
the "all" in the name is now slightly weird
There was a problem hiding this comment.
Yeahhhh, I thought about that too. Just given that this is Ruby and everything is allowed to call everything, I opted just to leave it.
The name is still somewhat apt because you can have multiple connection managers for a single config, so you can read this as "clear all the connection managers for config X".
|
Thanks Richard! |
remi-stripe
deleted the
brandur-only-clear-connection-managers-for-changed-config
branch
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.
@joeltaylor We've foisted enough work on you already, but if you'd like, feel free to take a look at this change set to make sure it's in-line with your implementation's principles.