Allow StripeClient to be configured per instance #968

joeltaylor · 2021-03-25T03:08:48Z

This changes allows for each instance of StripeClient to have its own
configuration object instead of relying on the global config. Each
instance can be configured to override any global config values
previously set.

brandur-stripe

Amazing @joeltaylor!! Left a couple comments below (Rubocop's probably going to make you change some things back), but this looks great.

lib/stripe/connection_manager.rb

brandur-stripe · 2021-03-25T18:22:04Z

lib/stripe/stripe_client.rb

+      when Stripe::ConnectionManager
+        {}
+      when String
+        { api_key: config_overrides }


Is this just a convenience use? I wonder if we should just make them be explicit.

I added this based on a recommendation from @ob-stripe (#954 (comment)). I'm happy to go either way on it.

Ah oops. Admittedly, this was a bit of a bike shed opinion, so okay, happy to go either way.

brandur-stripe · 2021-03-26T17:59:28Z

lib/stripe/stripe_client.rb

      current_thread_context.default_connection_manager ||= begin
-        connection_manager = ConnectionManager.new
+        connection_manager = ConnectionManager.new(config)


There's a potential for a bug here if a StripeClient starts making requests with one configuration, then another is initialized on the same thread with a different configuration, but still inherits the ConnectionManager of the first client.

I'm seeing two possibilities:

Error if we detect that configuration is different. This has the substantial downside though of not allowing two active StripeClients with different configurations to share the same thread.

Change ThreadContext#default_connection_manager to a map, and key connect managers to a map keyed to configuration. e.g. current_thread_context.default_connection_managers[config] = ....

Aye, that's a great catch. I completely missed ||= there.

I'm in favor of option two. Seems like it would better satisfy the type of functionality that's requested in #966.

@brandur-stripe I've implemented the second approach and added a couple of specs to capture any regressions for the bug.

At first, I used the configuration's object_id to key the connection managers, but realized that could potentially increase the memory footprint when configuration objects are identical and aren't eligible for manual GC. I decided to opt for a deterministic key here:

stripe-ruby/lib/stripe/stripe_configuration.rb

Lines 188 to 193 in 190052b

def key

@key ||= begin

instance_variables

.collect { |variable| instance_variable_get(variable) }

.join

end

An alternative would be to use Marshal.dump, but I'm a bit skeptical about the performance (I haven't benchmarked, but I'm more than happy to.)

CLAassistant · 2021-03-29T01:54:36Z

All committers have signed the CLA.

joeltaylor · 2021-03-29T02:00:30Z

lib/stripe/resources/account.rb

-    def self.retrieve(id = ARGUMENT_NOT_PROVIDED, opts = {})
-      id = if id.equal?(ARGUMENT_NOT_PROVIDED)
-             nil
-           else
-             Util.check_string_argument!(id)
-           end


This PR is a chunk of work pulled out of #954. For context on this change:

ARGUMENT_NOT_PROVIDED doesn't play very well with how we merge options. The args object in method_missing is an array and merging options when an ID is omitted results in [nil, {}] being sent to the resource, which isn't desirable.

However, I wasn't able to re-create any of the scenarios that warranted the addition of it in c269e9b and believe it's safe for removal.

#954 (comment)

Thx for checking those. Not sure if it there was something different going on when we originally put this in, but re-reading it again, it looks safe to take it out `ARGUMENT_NOT_PROVIDED.

brandur-stripe

@joeltaylor This is beautiful! Thanks. Left a couple more minor comments around the connection manager pruning process, but thank you so much for modifying that to make it multi-client friendly it.

brandur-stripe · 2021-03-29T17:34:57Z

lib/stripe/stripe_client.rb

@@ -345,6 +366,17 @@ class ThreadContext
      # because that's wrapped in an `ensure` block, they should never leave
      # garbage in `Thread.current`.
      attr_accessor :last_responses
+
+      # A default `ConnectionManager` for the thread. Normally shared between


Mind updating this comment to just refer to these in the plural?

brandur-stripe · 2021-03-29T17:39:59Z

lib/stripe/stripe_client.rb

+            next if connection_manager.last_used > last_used_threshold
+
+            connection_manager.clear
+            thread_context.reset_connection_managers


Shouldn't we only remove the connection manager that we're currently looping over rather than all the ones on the thread?

Good call! I'll update this to clear the single connection manager.

brandur-stripe · 2021-03-29T17:40:51Z

lib/stripe/stripe_client.rb

+
+            connection_manager.clear
+            thread_context.reset_connection_managers
+            pruned_thread_contexts << thread_context


And similarly, I wonder if we should only prune the thread context once all connect managers in it have confirmed to not be in use. This might be easiest to do by just introducing a second loop that runs after we prune all eligible connection managers.

Sounds reasonable to me. I'll get that update added 👍

Excellent! Thx again.

joeltaylor · 2021-03-30T16:22:36Z

lib/stripe/stripe_client.rb

@@ -9,19 +9,35 @@ module Stripe
  class StripeClient
    # A set of all known thread contexts across all threads and a mutex to
    # synchronize global access to them.
-    @thread_contexts_with_connection_managers = []
+    @thread_contexts_with_connection_managers = Set.new


Updated this to be a Set so prevent duplicate thread contexts from being added.

This changes allows for each instance of StripeClient to have its own configuration object instead of relying on the global config. Each instance can be configured to override any global config values previously set.

joeltaylor · 2021-03-30T16:31:53Z

test/stripe/stripe_client_test.rb

+        assert_equal({}, StripeClient.current_thread_context.default_connection_managers)
+      end
+
+      should "only garbage collect when all connection managers for a thread are expired" do


@brandur-stripe added this spec to capture the behavior of waiting for all connection managers to be expired before GCing a thread_context.

Side note: I got bit pretty hard when writing this spec because I created the second StripeClient with the config override {read_timeout: 100} without realizing that some configuration options will call StripeClient.clear_all_connection_managers. I wonder if that's something worth pointing out in the README since it could adversely affect those trying to get bleeding performance?

Thanks! And very nice — the connect manager management logic looks great now.

Side note: I got bit pretty hard when writing this spec because I created the second StripeClient with the config override {read_timeout: 100} without realizing that some configuration options will call StripeClient.clear_all_connection_managers. I wonder if that's something worth pointing out in the README since it could adversely affect those trying to get bleeding performance?

Oof :/ Actually, the fact that this happens now should be considered a bug. The reason that setting those configurations clears config is that we previously only had a single configuration object, and when people set a new global configuration like Stripe.api_base = , it was appropriate to clear out any existing clients. Now that we have specific client configurations, the preferable way of doing that instead would be just to create a new StripeClient with a different configuration. I suppose we could also support changing configuration per-client, but in that case we should only clear out connect managers relevant to that specific config object.

I'll bring in your PR first and take a stab at fixing this separately.

brandur-stripe · 2021-04-01T21:19:49Z

Thanks again for helping to get this in Joel!

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.

…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>

This is just a cosmetic change that renames `Stripe.configuration` to just `Stripe.config`. We use the shorter "config" in most other places including `StripeClient#config`, so I feel that this is overall more consistent. This change is backwards compatible because the new accessor came in with #968, and that hasn't been given a formal release yet. I've left the class name as `StripeConfiguration` which IMO is fine. The class uses the expanded form of the name while vars and accessors use the shorter `config`. Also, `StripeConfiguration` has been around a little bit longer, so renaming it is somewhat backwards incompatible too.

brandur-stripe · 2021-04-02T20:28:58Z

Released as 5.31.0.

joeltaylor force-pushed the configurable-client branch from 24f6a12 to e012990 Compare March 25, 2021 03:12

brandur-stripe reviewed Mar 26, 2021

View reviewed changes

joeltaylor force-pushed the configurable-client branch 3 times, most recently from bd37e7e to 2da0ffa Compare March 29, 2021 01:54

joeltaylor force-pushed the configurable-client branch from 2da0ffa to df32b93 Compare March 29, 2021 01:57

joeltaylor commented Mar 29, 2021

View reviewed changes

joeltaylor force-pushed the configurable-client branch from df32b93 to 190052b Compare March 29, 2021 02:04

joeltaylor marked this pull request as ready for review March 29, 2021 02:10

brandur-stripe reviewed Mar 29, 2021

View reviewed changes

joeltaylor force-pushed the configurable-client branch from 190052b to 588dd93 Compare March 30, 2021 16:21

joeltaylor commented Mar 30, 2021

View reviewed changes

Allow StripeClient to be configured per instance

344fa7a

This changes allows for each instance of StripeClient to have its own configuration object instead of relying on the global config. Each instance can be configured to override any global config values previously set.

joeltaylor force-pushed the configurable-client branch from 588dd93 to 344fa7a Compare March 30, 2021 16:24

joeltaylor commented Mar 30, 2021

View reviewed changes

brandur-stripe merged commit 21643f0 into stripe:master Apr 1, 2021

brandur-stripe mentioned this pull request Apr 1, 2021

On config change, only clear connection managers for changed config #971

Merged

brandur mentioned this pull request Apr 2, 2021

Rename Stripe.configuration to Stripe.config #972

Merged

excid3 mentioned this pull request Apr 29, 2021

How can we make the gem to be dynamic to multiple processor accounts in the same app? pay-rails/pay#316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow StripeClient to be configured per instance #968

Allow StripeClient to be configured per instance #968

joeltaylor commented Mar 25, 2021

brandur-stripe left a comment

brandur-stripe Mar 25, 2021

joeltaylor Mar 27, 2021

brandur-stripe Mar 29, 2021

brandur-stripe Mar 26, 2021

joeltaylor Mar 27, 2021 •

edited

joeltaylor Mar 29, 2021

CLAassistant commented Mar 29, 2021 •

edited

joeltaylor Mar 29, 2021

brandur-stripe Mar 29, 2021

brandur-stripe left a comment

brandur-stripe Mar 29, 2021

brandur-stripe Mar 29, 2021

joeltaylor Mar 29, 2021

brandur-stripe Mar 29, 2021

joeltaylor Mar 29, 2021

brandur-stripe Mar 29, 2021

joeltaylor Mar 30, 2021

brandur-stripe Apr 1, 2021

joeltaylor Mar 30, 2021

brandur-stripe Apr 1, 2021

brandur-stripe commented Apr 1, 2021

brandur-stripe commented Apr 2, 2021

	def key
	@key \|\|= begin
	instance_variables
	.collect { \|variable\| instance_variable_get(variable) }
	.join
	end

Allow StripeClient to be configured per instance #968

Allow StripeClient to be configured per instance #968

Conversation

joeltaylor commented Mar 25, 2021

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeltaylor Mar 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Mar 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Apr 1, 2021

brandur-stripe commented Apr 2, 2021

joeltaylor Mar 27, 2021 •

edited

CLAassistant commented Mar 29, 2021 •

edited