From 8f81e6a9bc1d3f49da15dffcb4d26ec7e8a8a9dc Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 19 Aug 2019 14:38:41 -0700 Subject: [PATCH] Do better bookkeeping when tracking state in `Thread.current` This is largely just another cleanup patch, but does a couple main things: * Hoists the `last_response` value into thread state. This is a very minor nicety, but effectively makes `StripeClient` fully thread-safe, which seems like a minor nicety. Two calls to `#request` to the same `StripeObject` can now be executed on two different threads and their results won't interfere with each other. * Moves state off one-off `Thread.current` keys and into a single one for the whole client which stores a new simple type of record called `ThreadContext`. Again, this doesn't change much, but adds some minor type safety and lets us document each field we expect to have in a thread's context. --- lib/stripe/stripe_client.rb | 62 ++++++++++++++++++++++++++----- test/stripe/stripe_client_test.rb | 11 +++--- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 9714482dd..d451c45e6 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -21,17 +21,23 @@ def initialize(connection_manager = nil) @last_request_metrics = nil end - # For internal use: sets a value on the current thread's store when + # Gets a currently active `StripeClient`. Set for the current thread when # `StripeClient#request` is being run so that API operations being executed # inside of that block can find the currently active client. It's reset to # the original value (hopefully `nil`) after the block ends. + # + # For internal use only. Does not provide a stable API and may be broken + # with future non-major changes. def self.active_client - Thread.current[:stripe_client] || default_client + current_thread_context.active_client || default_client end # Finishes any active connections by closing their TCP connection and # clears them from internal tracking in all connection managers across all # threads. + # + # 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 # Just a quick path for when configuration is being set for the first # time before any connections have been opened. There is technically some @@ -45,13 +51,13 @@ def self.clear_all_connection_managers # A default client for the current thread. def self.default_client - Thread.current[:stripe_client_default_client] ||= + current_thread_context.default_client ||= StripeClient.new(default_connection_manager) end # A default connection manager for the current thread. def self.default_connection_manager - Thread.current[:stripe_client_default_connection_manager] ||= begin + current_thread_context.default_connection_manager ||= begin connection_manager = ConnectionManager.new @all_connection_managers_mutex.synchronize do @@ -121,15 +127,16 @@ def self.sleep_time(num_retries) # charge, resp = client.request { Charge.create } # def request - @last_response = nil - old_stripe_client = Thread.current[:stripe_client] - Thread.current[:stripe_client] = self + old_stripe_client = self.class.current_thread_context.active_client + self.class.current_thread_context.active_client = self + self.class.current_thread_context.last_response = nil begin res = yield - [res, @last_response] + [res, self.class.current_thread_context.last_response] ensure - Thread.current[:stripe_client] = old_stripe_client + self.class.current_thread_context.active_client = old_stripe_client + self.class.current_thread_context.last_response = nil end end @@ -197,7 +204,7 @@ def execute_request(method, path, end # Allows StripeClient#request to return a response object to a caller. - @last_response = resp + self.class.current_thread_context.last_response = resp [resp, api_key] end @@ -248,6 +255,41 @@ def execute_request(method, path, }.freeze private_constant :NETWORK_ERROR_MESSAGES_MAP + # A record representing any data that `StripeClient` puts into + # `Thread.current`. Making it a class likes this gives us a little extra + # type safety and lets us document what each field does. + # + # For internal use only. Does not provide a stable API and may be broken + # with future non-major changes. + class ThreadContext + # A `StripeClient` that's been flagged as currently active within a + # thread by `StripeClient#request`. A client stays active until the + # completion of the request block. + attr_accessor :active_client + + # A default `StripeClient` object for the thread. Used in all cases where + # the user hasn't specified their own. + attr_accessor :default_client + + # A default `ConnectionManager` for the thread. Normally shared between + # all `StripeClient` objects on a particular thread, and created so as to + # minimize the number of open connections that an application needs. + attr_accessor :default_connection_manager + + # A response from the last executed API call. Used to return a response + # from a call to `StripeClient#request`. + attr_accessor :last_response + end + + # Access data stored for `StripeClient` within the thread's current + # context. Returns `ThreadContext`. + # + # For internal use only. Does not provide a stable API and may be broken + # with future non-major changes. + def self.current_thread_context + Thread.current[:stripe_client__internal_use_only] ||= ThreadContext.new + end + private def api_url(url = "", api_base = nil) (api_base || Stripe.api_base) + url end diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index f24e85f34..a69099079 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -826,14 +826,15 @@ class StripeClientTest < Test::Unit::TestCase should "reset local thread state after a call" do begin - Thread.current[:stripe_client] = :stripe_client + StripeClient.current_thread_context.active_client = :stripe_client client = StripeClient.new client.request {} - assert_equal :stripe_client, Thread.current[:stripe_client] + assert_equal :stripe_client, + StripeClient.current_thread_context.active_client ensure - Thread.current[:stripe_client] = nil + StripeClient.current_thread_context.active_client = nil end end end @@ -841,7 +842,7 @@ class StripeClientTest < Test::Unit::TestCase context "#proxy" do should "run the request through the proxy" do begin - Thread.current[:stripe_client_default_connection_manager] = nil + StripeClient.current_thread_context.default_connection_manager = nil Stripe.proxy = "http://user:pass@localhost:8080" @@ -857,7 +858,7 @@ class StripeClientTest < Test::Unit::TestCase ensure Stripe.proxy = nil - Thread.current[:stripe_client_default_connection_manager] = nil + StripeClient.current_thread_context.default_connection_manager = nil end end end