Add a couple of accessors

[ob-stripe]

Description

Add read accessors for Puma::Cluster's @workers and Puma::Runner's @options.

Somewhat related to #2773, these accessors would make it easier to implement a rolling restart strategy from a thread.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[dentarg]

Should we test these? Document them? I guess they will be part of Puma's public API?

[ob-stripe]

Attributes generally don't seem to be documented or tested throughout Puma's codebase, but I'm happy to add comments or tests if you think it's preferable.

(Also happy to close the PR if there are concerns around making those attributes part of the public API.)

[MSP-Greg]

I hate to ask, but are there particular options you're looking for? I think the public API's surface is too big, and adding the options hash to it...

Off-topic:

There are a few classes used as such:

t = SomeClass.new opts
t.meth1 opts[key1]
t.attr1 = opts[key2]
t.run

Drives me nuts...

[nateberkopec]

For context for everyone else, IIRC Stripe has a ton of patches to Puma. They're trying to upstream everything so they can get on mainline Puma.

@ob-stripe a use case would be good here.

[dentarg]

Attributes generally don't seem to be documented or tested throughout Puma's codebase

I think we should try to change that :-)

[ob-stripe]

Happy new year everyone! 🥳

I think we should try to change that :-)

Fair enough :)

@ob-stripe a use case would be good here.

The use case for these specific accessors is related to #2773: we need to regularly restart our Puma workers to avoid running into OOM issues (because our workers are bad and slowly leak memory over time). Using the change in #2773 + the accessors in this PR, it's fairly easy to write a thread in the main process that restarts workers every so often while ensuring we never have less than the nominal amount of workers at any time.

Here's a simplified version of the thread's main method that is executed every few seconds. @puma_cluster is the Puma::Cluster instance and @nominal_num_workers is the nominal number of workers, i.e. the number of workers that was set in the configuration at startup.

   def thread_run_iteration
      # Do nothing unless all workers are running.
      return unless @puma_cluster.workers.all? {|w| w.last_status[:running] == 1}

      # Get the current number of workers, and store it in a variable (on the off chance it
      # changes during the execution of this method).
      current_num_workers = @puma_cluster.options[:workers]

      if current_num_workers > @nominal_num_workers
        # Puma is currently configured to have more workers than the nominal number of workers.

        # Reduce the number of workers. Puma will detect this and gracefully terminate the excess
        # workers, starting with the oldest (assuming we've configured `worker_culling_strategy` to
        # `oldest`).
        log.info("PUMA-WORKER-ROLLING-RESTART: Setting number of workers back to #{@nominal_num_workers} (nominal value)")

        @puma_cluster.options[:workers] = @nominal_num_workers

      else
        # Puma is currently configured to have the nominal number of workers.

        # Check if there any workers old enough to be restarted. If not, do nothing.
        num_old_workers = @puma_cluster.options[:workers].count {|w| w.started_at < Time.now - MIN_WORKER_AGE}
        return if num_old_workers == 0

        # Increase the number of workers. Puma will automatically start the additional workers.
        new_num_workers = current_num_workers + [num_old_workers, @max_additional_workers].min
        log.info("PUMA-WORKER-ROLLING-RESTART: Increasing number of workers to #{new_num_workers}")
        @puma_cluster.options[:workers] = new_num_workers
      end
    end

As an alternative to exposing the attributes, I could also submit another PR to build the "worker rolling restart" feature directly in Puma if that's something you think might be valuable to other Puma users.

Let me know which you'd prefer. I'd also understand if neither of these proposals fit with your vision of Puma -- in this case I'm happy to keep using monkey-patches and leave extensive comments in my code to make it clear that it depends on private Puma internals and needs to be checked and possibly updated every time we update Puma (as @nateberkopec mentioned, we sadly have a bunch of code like this already...).

[nateberkopec]

I could also submit another PR to build the "worker rolling restart" feature directly in Puma

That's already been implemented quite well in puma_worker_killer so I don't see a need to mainline it.

I can't quite figure out how @schneems is reading the worker count in that gem though...

[ob-stripe]

I can't quite figure out how @schneems is reading the worker count in that gem though...

I think they're:

1. using ObjectSpace to find the Puma::Cluster object (there should be only one): https://github.com/schneems/puma_worker_killer/blob/8107b090212e64a1415c16c94a3907cc4e27f4ae/lib/puma_worker_killer/puma_memory.rb#L67
2. using instance_variable_get to access the private workers variable: https://github.com/schneems/puma_worker_killer/blob/8107b090212e64a1415c16c94a3907cc4e27f4ae/lib/puma_worker_killer/puma_memory.rb#L74

[nateberkopec]

OK yeah, that's jank. I'm in favor of adding this, then.

[nateberkopec]

So if we add comments/tests as appropriate (can obviously be very simple in both cases) this is OK to merge.

[ob-stripe]

Wonderful. Added some comments (description + YARD type info).