Multiple Client Instances & Orphaned Threads

[tonyta]

We ran into this issue where instantiating a client can lead to an orphaned thread (and possibly multiple persistent connection to the LD server). We've found a workaround for our implementation, but it seems like unintended behavior. Hope this helps!

Describe the bug

LaunchDarkly::Client seems to leave orphaned threads if client instances are garbage collected before closing.

We originally encountered this in Rails, when autoloading reloading made us lose the reference to our original client and we instantiated another one. We noticed that there were multiple connections to LaunchDarkly involving multiple threads. These threads and connections remain until the ruby process is closed.

To reproduce

Here is a contrived reproduction script:

require "ldclient-rb"

puts "Ruby Version: #{RUBY_VERSION}"
puts "LaunchDarkly Version: #{LaunchDarkly::VERSION}"

puts
puts "Creating 10 clients..."
clients = Array.new(10) { LaunchDarkly::LDClient.new("") }
puts "clients: #{ObjectSpace.each_object(LaunchDarkly::LDClient).count}"
puts "threads: #{Thread.list.count}"

puts
puts "Closing every other client..."
clients.each_with_index { |c, i| c.close if i.even? }
puts "clients: #{ObjectSpace.each_object(LaunchDarkly::LDClient).count}"
puts "threads: #{Thread.list.count}"

puts
puts "Clearing client references and garbage collecting..."
clients.clear
GC.start
sleep 1
puts "clients: #{ObjectSpace.each_object(LaunchDarkly::LDClient).count}"
puts "threads: #{Thread.list.count}"

puts
puts "Remaining threads..."
puts Thread.list

The above script initializes 10 clients. The threads created by the closed clients are eventually terminated. The threads created by the other clients remain indefinitely even after those clients are garbage collected.

Note: I'm aware that I'm initializing with an invalid SDK key, but this didn't seem to have an effect on the outcome.

Ruby Version: 2.5.5
LaunchDarkly Version: 5.6.0

Creating clients...
E, [2019-10-24T15:48:44.765951 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:44.766084 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:45.105761 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:45.105904 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:45.399326 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:45.399652 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:45.729868 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:45.730277 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:46.132979 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:46.133164 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:46.436607 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:46.436749 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:46.727070 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:46.727248 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:47.055064 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:47.055177 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:47.464489 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:47.464632 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
E, [2019-10-24T15:48:47.774941 #41465] ERROR -- : [LDClient] HTTP error 401 (invalid SDK key) for streaming connection - giving up permanently
E, [2019-10-24T15:48:47.775231 #41465] ERROR -- : [LDClient] LaunchDarkly client initialization failed
clients: 10
threads: 12

Closing every other client...
clients: 10
threads: 7

Clearing client references and garbage collecting...
clients: 0
threads: 7

Remaining threads...
#<Thread:0x00007fdae4065c40 run>
#<Thread:0x00007fdae392a1b0@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/concurrent-ruby-1.1.5/lib/concurrent/executor/ruby_thread_pool_executor.rb:317 sleep>
#<Thread:0x00007fdae40fb880@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/launchdarkly-server-sdk-5.6.0/lib/ldclient-rb/events.rb:172 sleep_forever>
#<Thread:0x00007fdae3601060@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/launchdarkly-server-sdk-5.6.0/lib/ldclient-rb/events.rb:172 sleep_forever>
#<Thread:0x00007fdae35d3408@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/launchdarkly-server-sdk-5.6.0/lib/ldclient-rb/events.rb:172 sleep_forever>
#<Thread:0x00007fdae358ad98@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/launchdarkly-server-sdk-5.6.0/lib/ldclient-rb/events.rb:172 sleep_forever>
#<Thread:0x00007fdae3550030@/Users/tonyta/.rbenv/versions/2.5.5/lib/ruby/gems/2.5.0/gems/launchdarkly-server-sdk-5.6.0/lib/ldclient-rb/events.rb:172 sleep_forever>

Expected behavior
If the expected use is to have a single client instance, this gem take a more control of its client initialization (a la newrelic_rpm), with stricter control over its threads.

Otherwise, if multiple clients are expected, it'd be nice to keep track and terminate orphaned threads. I'm not sure if multiple connections to the LaunchDarkly server is created, but is so, those should be closed too.

Logs
none

SDK version
LaunchDarkly Version: 5.6.0

Language version, developer tools
Ruby Version: 2.5.5

OS/platform
Platform: x86_64-darwin18

Additional context
none

[eli-darkly]

I'm not very familiar with Rails autoloading behavior— are you saying it's not possible for you to explicitly close a client that you know you're not going to be able to hold on to?

Off the top of my head, one way we might deal with this would be to define a finalizer for the client object which would ensure that the close logic runs if it's garbage-collected.

[tonyta]

I misspoke... I meant Rails reloading not autoloading (corrected above). In development mode, Rails can reload classes that have changed.

We figured out a different implementation to avoid this issue, but the original problem came about when we refined an autoloadable/reloadable class like the following:

class Feature
  class << self
    def ld_client
      @ld_client ||= LaunchDarkly::LDClient.new(ENV["LD_SDK_KEY"])
    end
  end
end

When Rails reloads the Feature class, Ruby garbage-collects the original Feature class and defines a new one with a new instance variable @ld_client. This means the original client also gets garbage collected and its thread is orphaned and sleeps forever and a new client is lazily created. This means two threads and potentially two connections to LD.

Even though we've figured a better implementation for this, this could potentially be a gotcha for other users of this gem. You do have good documentation about this in your Ruby SDK Reference:

LDClient must be a singleton

It's important to make this a singleton-- internally, the client instance maintains internal state that allows us to serve feature flags without making any remote requests. Be sure that you're not instantiating a new client with every request.

But it might be helpful if the LD gem manages the client singleton internally to avoid improper implementation.

[eli-darkly]

Unfortunately, I don't think the idea of having the LD gem manage a singleton client will work. There are sometimes cases where you really do want to have two clients (e.g. if you are using feature flags from multiple environments) so the server-side SDKs always allow you to create multiple instances; we just don't want people to create them unnecessarily every single time there is a new web request. We've found that the only way to reliably discourage that mistake is to make a very strong statement about how you should use a singleton instance. So really "must" in that case means "must (in almost all use cases)".

[eli-darkly]

Something I'm unclear on in your example: what does Feature represent? There isn't more than one of them, is there?

[louis-launchdarkly]

Given there is no comment from the requester in the past two years, closing this ticket. If you need instructions on how to close the client when a worker terminates, please refer to #144.