launchdarkly / ruby-server-sdk

LaunchDarkly Server-side SDK for Ruby
https://docs.launchdarkly.com/sdk/server-side/ruby
Other
34 stars 52 forks source link

Clarify SDK initialization instructions for rails applications #144

Closed ohsabry closed 1 year ago

ohsabry commented 4 years ago

The SDK documentation says that for a rails application, creating an instance of the client in an initializer, eg config/initializers/launchdarkly.rb:

https://docs.launchdarkly.com/docs/ruby-sdk-reference#section-initializing-ldclient-in-a-rails-application

However, it does then mention instructions for different servers like unicorn, puma, etc where reinitialises the client when a worker is forked

Similarly, with Unicorn, you'll need to specify an after_fork hook in your unicorn.rb config file:

after_fork do |server,worker|
Rails.configuration.ld_client = LaunchDarkly::LDClient.new('SDK KEY')
end

Is it necessary to reinitialize the client on every worker fork? If so, why?

eli-darkly commented 4 years ago

You wouldn't be _re_initializing the client on every fork, but rather constructing it for the first time only after the fork starts. When a process is forked, the forked process gets an exact copy of every object that already existed in memory— but that won't work correctly if the object is maintaining active network connections, as the SDK client does.

ohsabry commented 4 years ago

@eli-darkly ah yes makes sense - I forgot that the SDK keeps a persistent connection. Am I correct in assuming that if I initialise in after_fork, i don't need to construct the object in config/initializers/launchdarkly.rb?

eli-darkly commented 4 years ago

Yes, I think the wording of the documentation was unclear there but it means that you should do that instead of the default procedure described at the top.

grzuy commented 3 years ago

it means that you should do that instead of the default procedure described at the top.

Hi @eli-darkly ,

I might be missing something here probably, but wouldn't that mean that you won't have a LD client initialized in a rails console (or a rake task and other non-server boots), if you remove the config/initializers/launch_darkly.rb altogether and only keep it initialized in a config/puma.rb e.g.?

rod-murphy commented 3 years ago

I might be missing something here probably, but wouldn't that mean that you won't have a LD client initialized in a rails console (or a rake task and other non-server boots), if you remove the config/initializers/launch_darkly.rb altogether and only keep it initialized in a config/puma.rb e.g.?

We implemented the following in config/initializers/launch_darkly.rb to get around that:

if !defined?(::Rails::Server)
  Rails.application.config.ld_client = LaunchDarkly::LDClient.new('SDK KEY', LaunchDarkly::Config.default, 10)
end

::Rails::Server should always be defined in a puma/unicorn worker

ryanolds-drizly commented 2 years ago

Related question. If we are constructing a client for the worker/fork, do we also need to close that client when the worker exits/shutsdown?

We are using Puma and it looks like we are creating a new client for each worker, so it stands to reason that we would need to close the clients when the workers shutdown.

louis-launchdarkly commented 2 years ago

@ryanolds-drizly While I am not a specialist on how Puma web servers reclaim memories and resources, I believe closing the client will help memories and resources reclamation. So I recommend closing the client when the worker exits/shutdown, even when it may still works fine without explicitly closing it.

Our SDK Documentation: Shutdown Ruby does not currently call out the case when each fork/worker initialize its own client. We will update our documentation to clarify the behavior.

disposedtrolley commented 2 years ago

Our SDK Documentation: Shutdown Ruby does not currently call out the case when each fork/worker initialize its own client. We will update our documentation to clarify the behavior.

@louis-launchdarkly what's the correct method for shutting down individual clients in each worker process?

louis-launchdarkly commented 2 years ago

Based on Puma's Documentation about worker lifecycle, you can pass a block to the on_worker_shutdown and call close on the client reference inside the block. Because forking copies the object, calling close on the copy should not interfere with any other still running workers.

disposedtrolley commented 2 years ago

Thanks @louis-launchdarkly! Do you have any guidance for Unicorn-based Rails applications? They don't seem to provide a shutdown hook attached to each worker.

louis-launchdarkly commented 2 years ago

@disposedtrolley From the unicorn documentation, if you are using Unicorn 5.3.0+, there is an after_worker_exit block in which the worker is an argument to the block. You can call close there. I am not sure what is possible if you use a version before 5.3.0. Please let us know does that work for you.

And based on the discussion in https://github.com/launchdarkly/ruby-server-sdk/issues/143, my recommendation is to "Please call close on the Client when the worker shutdown or it could leave an orphaned thread behind."

disposedtrolley commented 2 years ago

@louis-launchdarkly the after_worker_exit block appears to be called by the Unicorn master rather than the workers, so I don't believe it has access to the singleton client that was initialised by the worker.

A lot of this is coming down to my lack of knowledge on how Unicorn servers perform process forking -- is it possible to access objects held by workers from the master, within that after_worker_exit block?

louis-launchdarkly commented 2 years ago

@disposedtrolley Sorry for missing your reply - based on the unicorn code signature, while it is called by the master, the code seems to still have the reference to the worker. So if your worker extends the https://github.com/defunkt/unicorn/blob/master/lib/unicorn/worker.rb class, you should be able to hold the reference that way.

jjb commented 1 year ago

regarding the discussion about closing clients when a puma worker shuts down - i'm not clear on the need here. a puma worker is an OS-level process. all resources will be gone when it shuts down. Or, is there a need to inform the LD service about this client going away, so overall statistics/sampling is correct?

prem-prakash commented 3 months ago

Same question here

julian-berbel commented 2 months ago

Documentation on initialization is still unclear. Having a separate section for Rails makes it seem as though the sections for specific web servers like Puma / Unicorn are only for projects that do not use rails (like sinatra based apps).