Allow Knock client to be used in multi-threaded Ruby applications #12
Conversation
Reusing the same Net::HTTP connection is not thread safe. To work around this, instantiate a new client per thread for use by that thread.
| @client.use_ssl = true | ||
|
|
||
| @client | ||
| Thread.current[:knock_client] ||= build_client |
There was a problem hiding this comment.
I think here one thing we might end up running into with this approach is open TCP connections that aren't cleaned up on the thread? It appears that the Stripe ruby client handles this by doing gc sweeps and trying to close open thread connections at the end of the request, but the approach is much more complex.
As a team, we're not the most familiar here with what the best approach is in regards to ruby and threading so are going to be looking for your guidance a bit here. Have you got any other examples of libraries you feel like handle this well?
There was a problem hiding this comment.
I think my advice for moving forward would be:
- Swap out Net::HTTP with Faraday -- it's an industry standard and lets implementors bring their own HTTP adapter for whatever they've already got in their infrastructure.
- Refactor the singleton so you can use this global pattern for simple cases, but also allows you to call
Knock::Client.newto avoid the global pattern. This would allow for connection cleanup by the caller if necessary. - If the global pattern sticks around, I'd still expect it to be thread safe. The Stripe approach does look quite complex, though I'd also have to do some digging to understand the repercussions of not cleaning up these TCP connections.
There was a problem hiding this comment.
I ran into this issue as well. I agree with @bmorton's suggestions. It would be great to either swap the client with Faraday or create a client instance per thread as a workaround for now.
I'm pretty sure the Net::HTTP client is creating a new connection every single time and closing it afterwards. That's the way it works by default. So this is already quite inefficient. See issues/19
There was a problem hiding this comment.
@cjbell I looked at Stripe's code that you linked before and they are keeping track of all connection managers inside a class instance variable. See https://github.com/stripe/stripe-ruby/blob/0890e1fb0fa70239ad8133469136e61e6091a24b/lib/stripe/stripe_client.rb#L10-L13
They need to garbage collect because if threads are created and then destroyed the StripeClient will continue to have references to the connections associated with those threads.
In @bmorton's change here the client is associated with the current thread and there is no reference to it in another class. This means that if a thread is destroyed the client will also be destroyed. So I don't think you need to do any GC or cleanup of connections like Stripe is doing. They have a much more complex setup.
Hey! We came across an issue where trying to use the Knock client was throwing exceptions under certain conditions and I was able to narrow it down to a threading issue. After looking at the code, reusing the same
Net::HTTPconnection is not thread safe. To work around this, I spiked out instantiating a new client per thread for use by that thread.Ultimately, I'd love to have something like Faraday in place so the HTTP adapter is swappable, too.