allow multiple after_worker_fork hooks

[doits]

fixes what seems to be a simple typo, now it behaves like expected

Do you think any test is needed for it?

[dentarg]

Tests would be great IMHO! Seeing existing tests did not catch this.

[doits]

Alright, I will give it a shot and add a test

[MSP-Greg]

@doits

A suggestion, a change to test/test_config.rb. Passes with your PR, fails in master with:

Errors & Failures:

  1) Failure:
TestConfigFile#test_run_hooks_after_worker_fork [/mnt/c/Greg/GitHub/puma/test/test_config.rb:305]:
--- expected
+++ actual
@@ -1 +1 @@
-["after_worker_fork is called with ARG_2"]
+["after_worker_fork is called with ARG_1", "after_worker_fork is called with ARG_2"]

  def assert_run_hooks(hook_name, options = {})
    configured_with = options[:configured_with] || hook_name

    # test single, not an array
    messages = []
    conf = Puma::Configuration.new
    conf.options[hook_name] = -> (a) {
      messages << "#{hook_name} is called with #{a}"
    }

    conf.run_hooks hook_name, 'ARG', Puma::Events.strings
    assert_equal messages, ["#{hook_name} is called with ARG"]

    # test multiple
    messages = []
    conf = Puma::Configuration.new do |c|
      c.send(configured_with) do |a|
        messages << "#{hook_name} is called with #{a}_1"
      end

      c.send(configured_with) do |a|
        messages << "#{hook_name} is called with #{a}_2"
      end

    end
    conf.load

    conf.run_hooks hook_name, 'ARG', Puma::Events.strings
    assert_equal messages, ["#{hook_name} is called with ARG_1", "#{hook_name} is called with ARG_2"]
  end

[doits]

ahh thanks @MSP-Greg, I just pushed my updated test, just checking that multiple blocks can be run and one without an argument, too.

I can change it to your version or keep it like this, I don't have a strong opinion here.

[MSP-Greg]

@doits

1. Thanks for the PR, and good catch.
2. I need more coffee (holiday, late start)

Parts of the code allow for either an array or a single object, normally a block, but it's always converted to an array. I thought testing for both might be a good idea, which is why I had two asserts (one for a single block, one for two).

The code to convert an object to an array is in Puma::UserFileDefaultOptions#all_of, but Puma::DSL also passes arrays. I updated the code above to 'manually' set the option to a lambda (not an array), so it's testing the logic in #all_of. Maybe better? Does fail on master...

[doits]

alright, I updated the test based on your comment, just a small rewording :-)

Didn't knew it is allowed/expected to manually set hooks like conf.options[:after_work_hook] = something etc., so good to test it if it is.

[MSP-Greg]

Didn't knew it is allowed/expected to manually set hooks

Part of the problem with API's is one could spend a month defining what is and isn't allowed. But, since the code in Puma::UserFileDefaultOptions#all_of did convert into an array, I thought testing it would be a good idea.

JFYI, a while ago I made a change to the API that I thought wouldn't cause an issue. It broke the code in a repo that needed a server for their CI, and they were starting Puma within code by creating a Puma::Server...

[doits]

Yeah, it will be done if it can be done ;-)

I think it is interesting that puma is not allowing to set multiple after_worker_fork hooks for 5 years already (introduced in 01b38fc). Looks like I am the only one who wants to use it ...?

[nateberkopec]

I think we set a speed record with this PR... 🚀