Add on_booted event #2709

rodzyn · 2021-09-22T14:13:09Z

Description

Closes #2635

I'm not sure if exactly that is what was intended. But I'm happy to get a feedback :)

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.

lib/puma/dsl.rb

lib/puma/single.rb

nateberkopec · 2021-09-22T14:19:57Z

Thank you so much for your first contrib to Puma! Welcome to the team!

rodzyn · 2021-09-22T17:16:34Z

Thank you so much for your first contrib to Puma! Welcome to the team!

Thanks for a warm welcome!

rodzyn · 2021-09-23T07:33:37Z

@nateberkopec One simple way that it could be achieved is to give a possibility for DSLs to have access to the events object. I did that in the latest commit

jacobherrington

I don't have much useful guidance on the implementation, but I was able to make the test you added pass with a change that might help you find the right path for this feature.

lib/puma/dsl.rb

test/test_config.rb

nateberkopec · 2021-12-12T20:16:50Z

Needs a test and I think it's good to go?

johnnyshields · 2022-01-11T13:55:26Z

lib/puma/dsl.rb

@@ -588,6 +592,17 @@ def after_worker_fork(&block)
      @options[:after_worker_fork] << block
    end

+    # Code to run after puma is booted in a single mode
+    #
+    # @note Single mode only.


Why this would be Single mode only? Cluster mode also calls fire(:on_booted)

Johnny is correct:

puma/lib/puma/cluster.rb

Line 468 in 5f255fc

@launcher.events.fire_on_booted!

puma/lib/puma/cluster.rb

Line 477 in 5f255fc

@events.fire_on_booted!

I guess the docs was written like this because the request (#2635, #1230 (comment)) came from a user using single mode where after_worker_boot (aliased to after_worker_fork) doesn't exist.

puma/lib/puma/cluster.rb

Line 104 in 5f255fc

@launcher.config.run_hooks :after_worker_fork, idx, @launcher.events

Thanks for catching this, correct it runs for both, clustered and single mode, I've changed the description. The question is, what would be the expected behavior? At the end the name of the event is on_booted so I think it makes sense to be run on both: single and clutered. Any thoughts @nateberkopec ?

dentarg · 2022-01-14T13:05:09Z

lib/puma/dsl.rb

@@ -251,6 +251,10 @@ def bind_to_activated_sockets(bind=true)
      @options[:bind_to_activated_sockets] = bind
    end

+    def events(events)
+      @options[:events] = events
+    end


Do we need to add events to the DSL? Should we document it? Looks like some kind of hack to me? :) Should it be a public thing in the DSL?

Should the other DSL objects also have access to @events?

puma/lib/puma/configuration.rb

Lines 146 to 148 in 5f255fc

@user_dsl = DSL.new(@options.user_options, self)

@file_dsl = DSL.new(@options.file_options, self)

@default_dsl = DSL.new(@options.default_options, self)

It was long time ago, but I think it was result of this discussion: #2709 (comment)

It was made as a temporary measure, to be refactored later I guess. I'm not sure if that is still right approach

I have changed the behavior, so now there is no need for this method and the Puma::Events instance is passed early to Puma::Configuration so it can be used by any type of DSL if needed

johnnyshields · 2022-02-06T11:08:36Z

We just merged #2798 which affects events, so please pull latest master to this branch.

axos88 · 2022-07-19T09:37:42Z

Can this be merged? I also need this functionality.

rodzyn · 2022-07-30T18:51:24Z

Sorry for abandoning this work for a long time. I had a very busy personal life in the past few months. I'm back with more time so I did a rebase. I will also try to look once again into a solution.

NickLarsenNZ · 2022-09-06T10:17:48Z

Any update on this?

indirect · 2022-11-08T07:03:10Z

If you don't have multiple processes, can't you just... run your code directly in puma.rb? For example, it seems like I have embedded Sidekiq working in a single non-worker Puma process like this:

# [the default rails puma.rb file goes here]

require "sidekiq"
require_relative "initializers/sidekiq"

sidekiq = Sidekiq.configure_embed do |config|
  # config.logger.level = Logger::DEBUG
  config.queues = %w[critical default low]
  config.concurrency = 2
end
sidekiq.run

at_exit do
  sidekiq&.stop
end

axos88 · 2023-01-12T21:55:49Z

Not necessarily. My use case involves running both single process and multi process based on environment. And I need the on booted event to fire correctly in both cases.

nateberkopec · 2023-01-23T05:16:45Z

Looks like you've done a git-whoopsie and deleted your test.

nateberkopec

Fix tests

rodzyn · 2023-02-07T10:08:41Z

@nateberkopec @dentarg - feel free to take a look if you have time whether these changes make sense

nateberkopec · 2023-02-20T01:52:38Z

Merged in 7c8cb02

nateberkopec · 2023-02-20T02:05:14Z

Crud, I screwed up the merge. @MSP-Greg can you take a look and try? the changes to rack/handler meant that my merge didn't work.

MSP-Greg · 2023-02-20T02:28:41Z

Looking at it now.

MSP-Greg · 2023-02-20T03:59:26Z

Test are running. I think the Windows mswin job will fail, it may be a recent change in Ruby master, re an error message. All the Actions master builds are done at different times, so they're not all the same commit in ruby/ruby...

I think the older msg is:

undefined method `to_i' for an instance of Array

newer:

undefined method `to_i' for [0, 1]:Array

MSP-Greg · 2023-02-20T04:46:57Z

Done...

nateberkopec · 2023-02-20T05:57:09Z

🙇 Thanks!

nateberkopec reviewed Sep 22, 2021

View reviewed changes

lib/puma/dsl.rb Outdated Show resolved Hide resolved

rodzyn force-pushed the on_booted_feature branch from d30be15 to 94e7d64 Compare September 22, 2021 14:14

nateberkopec reviewed Sep 22, 2021

View reviewed changes

lib/puma/single.rb Outdated Show resolved Hide resolved

nateberkopec added feature waiting-for-review Waiting on review from anyone labels Sep 23, 2021

jacobherrington reviewed Sep 29, 2021

View reviewed changes

lib/puma/dsl.rb Outdated Show resolved Hide resolved

test/test_config.rb Outdated Show resolved Hide resolved

rodzyn force-pushed the on_booted_feature branch from f0304fb to 6112487 Compare October 4, 2021 21:52

nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Dec 12, 2021

johnnyshields reviewed Jan 11, 2022

View reviewed changes

dentarg reviewed Jan 14, 2022

View reviewed changes

rodzyn force-pushed the on_booted_feature branch from 8e0f32c to 0799018 Compare July 30, 2022 18:11

nateberkopec mentioned this pull request Aug 26, 2022

Callbacks for app lifecycle? #2915

Closed

rodzyn force-pushed the on_booted_feature branch from daa138b to f4575cd Compare January 23, 2023 04:47

rodzyn requested review from dentarg, nateberkopec and johnnyshields and removed request for dentarg, nateberkopec and johnnyshields January 23, 2023 04:52

rodzyn requested a review from dentarg January 23, 2023 04:53

nateberkopec requested changes Jan 23, 2023

View reviewed changes

rodzyn force-pushed the on_booted_feature branch from cf01e9a to 33da1e1 Compare January 26, 2023 03:35

rodzyn changed the title ~~Add on_booted event for single mode~~ Add on_booted event Jan 26, 2023

rodzyn force-pushed the on_booted_feature branch from 33da1e1 to 1b7c021 Compare January 26, 2023 03:41

rodzyn requested review from nateberkopec and removed request for dentarg January 26, 2023 04:01

rodzyn force-pushed the on_booted_feature branch from 1b7c021 to 6ba097e Compare January 27, 2023 08:27

nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Jan 29, 2023

rodzyn force-pushed the on_booted_feature branch from 196f445 to 627261d Compare January 31, 2023 07:52

rodzyn force-pushed the on_booted_feature branch from 627261d to ca69608 Compare February 7, 2023 10:07

rodzyn force-pushed the on_booted_feature branch from ca69608 to e0fe05d Compare February 11, 2023 06:53

nateberkopec approved these changes Feb 20, 2023

View reviewed changes

nateberkopec closed this Feb 20, 2023

MSP-Greg reopened this Feb 20, 2023

Marcin Olichwirowicz and others added 2 commits February 19, 2023 20:40

Add on_booted event

5fd0869

Add on_booted support for Rack::Handler::Puma

babd019

MSP-Greg force-pushed the on_booted_feature branch from e0fe05d to babd019 Compare February 20, 2023 03:54

MSP-Greg merged commit a61b078 into puma:master Feb 20, 2023

dentarg mentioned this pull request Mar 29, 2024

"undefined method on_booted for nil:NilClass" on "pumactl start" #3356

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add on_booted event #2709

Add on_booted event #2709

rodzyn commented Sep 22, 2021 •

edited

nateberkopec commented Sep 22, 2021

rodzyn commented Sep 22, 2021

rodzyn commented Sep 23, 2021

jacobherrington left a comment

nateberkopec commented Dec 12, 2021

johnnyshields Jan 11, 2022

dentarg Jan 14, 2022

rodzyn Jan 23, 2023

dentarg Jan 14, 2022

dentarg Jan 14, 2022

rodzyn Jul 30, 2022

rodzyn Jan 23, 2023

johnnyshields commented Feb 6, 2022

axos88 commented Jul 19, 2022

rodzyn commented Jul 30, 2022

NickLarsenNZ commented Sep 6, 2022

indirect commented Nov 8, 2022

axos88 commented Jan 12, 2023

nateberkopec commented Jan 23, 2023

nateberkopec left a comment

rodzyn commented Feb 7, 2023

nateberkopec commented Feb 20, 2023 •

edited

nateberkopec commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

nateberkopec commented Feb 20, 2023

	@user_dsl = DSL.new(@options.user_options, self)
	@file_dsl = DSL.new(@options.file_options, self)
	@default_dsl = DSL.new(@options.default_options, self)

Add on_booted event #2709

Add on_booted event #2709

Conversation

rodzyn commented Sep 22, 2021 • edited

Description

Your checklist for this pull request

nateberkopec commented Sep 22, 2021

rodzyn commented Sep 22, 2021

rodzyn commented Sep 23, 2021

jacobherrington left a comment

Choose a reason for hiding this comment

nateberkopec commented Dec 12, 2021

johnnyshields Jan 11, 2022

Choose a reason for hiding this comment

dentarg Jan 14, 2022

Choose a reason for hiding this comment

rodzyn Jan 23, 2023

Choose a reason for hiding this comment

dentarg Jan 14, 2022

Choose a reason for hiding this comment

dentarg Jan 14, 2022

Choose a reason for hiding this comment

rodzyn Jul 30, 2022

Choose a reason for hiding this comment

rodzyn Jan 23, 2023

Choose a reason for hiding this comment

johnnyshields commented Feb 6, 2022

axos88 commented Jul 19, 2022

rodzyn commented Jul 30, 2022

NickLarsenNZ commented Sep 6, 2022

indirect commented Nov 8, 2022

axos88 commented Jan 12, 2023

nateberkopec commented Jan 23, 2023

nateberkopec left a comment

Choose a reason for hiding this comment

rodzyn commented Feb 7, 2023

nateberkopec commented Feb 20, 2023 • edited

nateberkopec commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

MSP-Greg commented Feb 20, 2023

nateberkopec commented Feb 20, 2023

rodzyn commented Sep 22, 2021 •

edited

nateberkopec commented Feb 20, 2023 •

edited