Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable persistent connections in the default client #590

Closed
jurriaan opened this issue Oct 12, 2017 · 6 comments
Closed

Enable persistent connections in the default client #590

jurriaan opened this issue Oct 12, 2017 · 6 comments

Comments

@jurriaan
Copy link

jurriaan commented Oct 12, 2017

Because the Stripe API is quite slow for us (we're based in Europe, and Stripe's servers are in the US) we played around with different Faraday adapters to be able to have persistent connections to at least get rid of the connection/SSL overhead. One of the adapters we tried is Patron. We noticed that Patron caused issues when running in a threaded environment.

I started looking at the StripeClient class to see how you handle this for the default client and connection.

As seen in the following snippet some work has been done to properly handle a threaded environment:

def self.default_client
@default_client ||= StripeClient.new(default_conn)
end
# A default Faraday connection to be used when one isn't configured. This
# object should never be mutated, and instead instantiating your own
# connection and wrapping it in a StripeClient object should be preferred.
def self.default_conn
# We're going to keep connections around so that we can take advantage
# of connection re-use, so make sure that we have a separate connection
# object per thread.
Thread.current[:stripe_client_default_conn] ||= begin

The default_conn is memoized on the current thread, which should make sure there are no threading issues. But because the default_client is not memoized on the current thread (it instead is memoized into a class instance variable), you will still get the same connection for all threads.

This is unfortunate, since this basically results in a non thread-safe client by default (Faraday does not guarantee thread-safety, clearly the Patron adapter isn't thread safe, because it reuses a single session which is not thread-safe), while the intention here clearly was to have a thread-safe client.

The easiest way to make the default client thread safe is to memoize it on the current thread as well. Another thing worth considering is moving away from Faraday and use a client preconfigured with persistent connections so everyone automatically benefits from these kinds of optimizations.

@ob-stripe
Copy link
Contributor

Hey @jurriaan, thanks a lot for the detailed report and proposed solutions! We'll take a look and get back to you quickly.

@brandur-stripe
Copy link
Contributor

#591 has been released as part of 3.5.1.

@stevenharman
Copy link

@brandur-stripe If this was fixed as part of 3.5.1, can this issue be closed?

@ob-stripe
Copy link
Contributor

The thread safety issue was fixed in 3.5.1, however we'd still like to implement persistent connections by default at some point, which is why we kept the issue open. I guess we ought to rename it to clarify though -- I'll do that now.

@ob-stripe ob-stripe changed the title The default stripe client is not necessarily thread safe Enable persistent connections in the default client Jan 24, 2018
@ob-stripe
Copy link
Contributor

ob-stripe commented Nov 16, 2018

Technically, this was implemented in 4.0.0 (cf. #698), however the gemspec file in that version is bad. This will be fixed in 4.0.2 which will be released later today.

@ob-stripe
Copy link
Contributor

Fixed in 4.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants