Add a 'worker_starting' event. #1755

rmanfredi · 2021-04-04T20:29:54Z

This event (before_app_start) is always called before the application starts, regardless
of whether it is invoked via morbo, is started in daemon mode from the
command line or via hypnotoad in pre-forking mode.

It avoids having to say:

Mojo::IOLoop->next_tick(sub ($ioloop) {
    app->log->info("Worker $$ star...ALL GLORY TO THE HYPNOTOAD!");
});

to get this special behaviour. It is way nicer to be able to say:

app->hook(before_app_start => sub ($app) {
    $app->log->info("App in $$ star...ALL GLORY TO MOJOLICIOUS!");
});

to get that behaviour.

It is more consistent with the over hooks, and also it is a better
abstraction than having to know the internals of the IOLoop to be
able to get such a hooking point in pre-forked processes.

Now the application has a unique and consistent entry point where it can
initialize its global state.

jhthorsen

I think I'm leaning towards thinking this might be a good addition since it is a question that comes up again and again. I don't like the event name though.

My review is indeed early, so I will come back to this PR when it has matured more.

jhthorsen · 2021-04-05T07:30:19Z

lib/Mojo/Server/Daemon.pm

@@ -35,6 +35,7 @@ sub run {
  my $loop = $self->ioloop;
  my $int  = $loop->recurring(1 => sub { });
  local $SIG{INT} = local $SIG{TERM} = sub { $loop->stop };
+  $self->app->worker_starting;


Isn't this "before_worker_start" ? And I wonder if we need a method for this...

@kraih: Is it bad to emit events from outside the app? As in $self->app->plugins->emit_hook(...)

Could rename the event as "before_worker_start" to be consistent with the other hooks, indeed!
I like it better myself.

@jhthorsen Yes, that's bad encapsulation.

@jhthorsen @kraih I am sorry, I do not follow your logic.

Why is it bad encapsulation to send advertised events to an object so that it can react appropriately if it wants? How is this violating encapsulation at all? Calling $app->app_starting is no more violating encapsulation than calling $app->server is. Or am I totally misunderstanding what you are talking about here?

Now you're crossing two layers instead of just one. The server knowing about a specific set of methods it can call on the app instance it manages is very different from it digging into very specific app properties that would normally only be reached via delegation by the app. It makes implementing new servers much much harder. Just because it is public doesn't make it part of the "server <-> app" protocol.

t/mojo/daemon.t

t/mojo/morbo.t

rmanfredi · 2021-04-05T08:48:59Z

Done with all the changes following the initial review.
Thank you @jhthorsen.

As of now, the hook is called "before_worker_start". However, I'm offering a new suggestion: "before_app_start".

Indeed, this is not a hook just for pre-forked servers, it is a generic hook that applications can use to initialize global state before they start processing requests. It just happens to also "do the right thing" in pre-forked environments.

kraih · 2021-04-05T12:55:25Z

Normally i'd be against this, since Mojo::IOLoop->next_tick works fine and has been the documented solution forever. But this also includes PSGI, so there might be enough original use cases here, but i'd really like to hear about at least two independent ones (same as we've done for every other hook before).

lib/Mojo/Server/Daemon.pm

lib/Mojolicious.pm

t/mojo/daemon.t

t/mojo/morbo.t

t/mojo/psgi.t

lib/Mojolicious/Guides/Cookbook.pod

rmanfredi · 2021-04-05T21:03:09Z

I believe this hook should really be named "before_app_start".

And I still need to write tests in t/mojo/prefork.t before I can close on this.

s1037989 · 2021-04-05T21:18:31Z

I believe this hook should really be named "before_app_start".

I like this name better. My only comment is just thinking about how this PR will ensure that the hook will get called for each worker start, even those [re]starting later; and my very possibly flawed reaction to before_app_start would imply that this hook code will get run once and only once (thinking #1694) -- i.e. before the app starts and starts forking off workers, but that is likely a distinction between the executable "app" and the Mojolicious "app", where the definition of "app" should indeed favor the latter, but possibly confusing nonetheless.

Also, a possible contradiction: is it really before_app_start if it is executed after app->start?

rmanfredi · 2021-04-05T21:24:48Z

Also, a possible contradiction: is it really before_app_start if it is executed after app->start?

Good point. But before_app_service would be ugly, would not it be? I would also ditch before_app_enters_ioloop 😄

Yeah, naming is really hard.

Grinnz · 2021-04-05T21:25:06Z

The name should reference that it is run on the start of each worker process, not the application. before_app_start would be misleading for both these reasons.

rmanfredi · 2021-04-05T21:27:59Z

The name should reference that it is run on the start of each worker process, not the application. before_app_start would be misleading for both these reasons.

I have made also patches to make sure the new hook is run before every possible application start I could see, so that behaviour can be controlled in a standard way: regardless of whether you are going to use pre-forking, or run in a single daemon, or via morbo, the aim is to have a hook to initialize global state that you would not want to do for every request you are going to serve...

Grinnz · 2021-04-05T21:31:12Z

But it is for every worker, which makes it different from "before application start", which only occurs once.

rmanfredi · 2021-04-05T21:34:38Z

My only comment is just thinking about how this PR will ensure that the hook will get called for each worker start, even those [re]starting later.

That's the idea. I can see that it works for my application, which already relies on the new hook here: the code gets triggered every time a new child is starting. My logging vouches for it 😄.

The trick is... how to write good tests for it. That's why I haven't touched t/mojo/prefork.t yet.

rmanfredi · 2021-04-05T21:38:45Z

But it is for every worker, which makes it different from "before application start", which only occurs once.

Yes, it is for every worker. That's why I had named it worker_starting initially, and then realized that when I used morbo it did not get called at all and that meant having another hook to do the same thing: getting the local process setup for efficiently processing incoming requests. So it's not just for pre-forked processes.

lib/Mojo/UserAgent/Server.pm

kraih · 2021-04-05T23:22:24Z

lib/Mojo/Server/CGI.pm

+  my $app   = $self->app;
+  $app->worker_starting if ref $app;
+  return $self;
+}


This is now inconsistent with before_server_start. Also a blocker for me.

How is it inconsistent exactly?

Mojo::Server::CGI is excluded from deployment hooks like before_server_start.

And this is also a deployment hook you are working on. It does not get emitted for commands like ./myapp.pl eval -V 'app->mode' after all.

@kraih you are right, something is not working as expected yet. I only discovered Mojo 2 days ago, hence I apologize for being a slow learner.
I really wish to have a global hook that triggers possibly costly initialization for the application, only once and regardless of how the application is actually started.

And that makes it inconsistent with before_server_start. Which does not run for ./myapp.pl cgi.

I think this is a blocker too. We might have to consider adding the before_server_start hook to cgi first.

Why do you try to link the fate of before_server_start with that of before_app_start? They have different semantics now.

We could add before_server_start to CGI, but that would be another PR, because, for me, the two events have different semantics, even if, at first, when I started toying with this idea, I emitted one close to the other...

Yes, i think before_server_start should be added to Mojo::Server::CGI after all.

And done. 81a2753...8e1611e

Pull request has been modified.

t/mojo/daemon.t

t/mojo/cgi.t

This event is always called before the application starts, regardless of whether it is invoked via morbo, is started in daemon mode from the command line, via hypnotoad in pre-forking mode, as a CGI application or as a PSGI. In pre-forked children, it avoids having to say: Mojo::IOLoop->next_tick(sub ($ioloop) { app->log->info("Worker $$ star...ALL GLORY TO THE HYPNOTOAD!"); }); to get this special behaviour. It is way nicer to be able to say: app->hook(before_app_start => sub ($app) { $app->log->info("App in $$ star...ALL GLORY TO MOJOLICIOUS!"); }); to get that behaviour. It is more consistent with the over hooks, and also it is a better abstraction than having to know the internals of the IOLoop to be able to get such a hooking point in pre-forked processes. Now the application has a unique and consistent entry point where it can initialize its global state, regardless of how it is being launched.

This is the standard width for that project.

rmanfredi · 2021-04-06T12:09:11Z

So, at this stage, I believe I have answered all the review comments so far. I'm letting you review the current changes now, awaiting more feedback.

Note that the semantics of the before_app_start event is to be emitted once per process (and per application), for global initialization by the application before it starts processing web requests, regardless of how exactly it is being run.

This means we do not need to evaluate code that could lie in ./myapp.pl before the app is actually started and is about to serve requests. The event actually becomes a global application initialization hook. That's the intent anyway.

kraih · 2021-04-06T12:19:06Z

lib/Mojo/Server/Daemon.pm

@@ -35,6 +35,7 @@ sub run {
  my $loop = $self->ioloop;
  my $int  = $loop->recurring(1 => sub { });
  local $SIG{INT} = local $SIG{TERM} = sub { $loop->stop };
+  $self->app->app_starting;


This should trigger twice now.

It did, hence the PID protection in Mojolicious.
If I remove this line, then morbo ./myapp.pl is not triggering the event!

Right, that's a different problem though. This is a newly designed feature, so it really shouldn't use workarounds like that.

kraih · 2021-04-06T12:20:54Z

lib/Mojolicious.pm

+  # Ensure we run 'before_app_start' only once per process
+  $self->{_started} //= {};
+  return if exists $self->{_started}->{$$};
+  $self->{_started}->{$$} = undef;


This should be completely unnecessary.

It's not. See above comment... If I do not do this, then there is a duplicate invocation (as you guessed) with having run() and start() both invoke the event...
Yet if both do not, then the event is not always emitted.

It's a workaround, not a solution for the actual problem (which is that morbo initialises the listener in the parent process before the application is ever loaded).

kraih · 2021-04-06T12:22:32Z

t/mojo/prefork.t

+    $counter_server->{$pid}++;
+    $c->rendered(201);
+  }
+);


Way too overcomplicated for a simple test.

Do the absolute minimum necessary to test the feature.

kraih · 2021-04-06T12:25:43Z

t/mojo/prefork.t

+
+is $foreign, 0, 'no before_app_start ran in foreign processes';
+
+kill 'TERM', $dpid;


This is a mess. Anything that risks making our fork tests less reliable is a blocker i'm afraid.

More constructively: Please make a self contained subtest (all new tests are required to use subtests), and make sure that it waits until the forked process is stopped safely.

Not familiar with the subtest thing. t/mojo/prefork.t did not use these.
Are you suggesting that I put all the preforking tests into a single subtest?

With my little webapp, I can collect statistics about all the tests done and make sure everything is working as expected: that callbacks are invoked the expected amount of time and in the right processes. Since we have many concurrent processes, I thought it would be nicer to cleanly abstract the statistics collection into a single tracking service that every sub-process knows how to reach and which has a simple REST API.

I can add a waitpid() call at the end to make sure we cleanup properly, but I have added a failsafe in the webapp by instantiating a callback every 0.5 second to check whether the child has been re-parented to init (sorry, one should say systemd nowadays). Did I do that right?

Please leave the existing tests alone, and treat everything new as a separate new subtest. We will convert the rest of prefork.t to subtests in time. Make the test app no more complicated than absolutely necessary. Every line that looks unnecessary will be questioned and might end up as a blocker.

The conversion is an ongoing process independent of this PR. #1520

kraih · 2021-04-06T12:32:28Z

t/mojo/daemon.t

+my $whatever = 'Failed!';
+$app->hook(
+  before_app_start => sub {
+    my ($app) = shift;


We don't use shift in list context.

Right. I meant ($app) = @_; but as your earlier comment indicates, you use $app = shift when there is just one argument expected.

kraih · 2021-04-06T12:33:22Z

t/mojo/daemon.t

+  before_app_start => sub {
+    my ($app) = shift;
+    my $ok = ref $app && $app->isa('Mojolicious');
+    $whatever = $ok ? 'Whatever!' : 'Wrong type';


Way too complicated again. Why have special case that is now untested?

What do you mean "untested"? This is tested indeed... if I change the 'Whatever!' string, the tests are failing!
Or did you mean something I don't get?

The Wrong type variant appears untested and rather pointless. You could just ref the app and use that as test value.

Or better yet, call a method in $app and use that, like ->moniker or so.

kraih · 2021-04-06T12:34:03Z

t/mojo/prefork.t

+);
+$prefork->app->hook(
+  before_app_start => sub {
+    my ($app) = @_;


It's not a list.

Aren't parameters always passed as a LIST in Perl? Even if there is a single one?

Yes, but we use the convention my $app = shift for single arguments. It became an idiom because it used to be a little faster with some versions of Perl (i don't actually remember the details, ask on IRC if you want to know more).

Yeah, I remember that discussion on p5p at the time. It was a loooong time ago. In my own code, I just don't care and prefer to say ($x) = @_; even if there is just one argument because I favor readability over hypothetical performance. My only exception is for my $self = shift; which I always use on a line by itself, even if there are following arguments.

But Mojolicious is not my code and I will follow your standards by amending this code once again.

If it was up to me alone we would just be using signatures everywhere. 😉

kraih · 2021-04-06T12:47:33Z

lib/Mojolicious.pm

+  $self->{_started} //= {};
+  return if exists $self->{_started}->{$$};
+  $self->{_started}->{$$} = undef;
+  $self->plugins->emit_hook(before_app_start => $self);


I find the new name misleading. It implies that it runs before app->start.

I agree here. In line with other comment from @s1037989 yesterday.
Naming is hard. What would you choose? before_app_serving? Or perhaps drop the before_ and say start? Or init?

after_server_start, consistent with before_server_start? 😁

Sure, but that would mean it has comparable semantics to some "server starting" event and... remember... there is no such thing for CGI yet by your own admission... 😀

That being said, I have absolutely no stake in this naming. I just want to have this new functionality in standard Mojolicious and not just in my own forked framework. Forking Open-Source projects is always... bad.

Note that Mojolicious gets forked so much that we had to adopt an official policy for it. https://docs.mojolicious.org/Mojolicious/Guides/Contributing#FORK-POLICY

Pull request has been modified.

mergify · 2021-04-06T18:39:26Z

This pull request is now in conflicts. Could you fix it @rmanfredi? 🙏

kraih · 2021-04-06T21:16:19Z

I've been thinking about this for a bit, and there are more edge cases that have not been considered yet. The best solution i can think of would be something like:

diff --git a/lib/Mojo/Server/Daemon.pm b/lib/Mojo/Server/Daemon.pm
index f85982841..98ddcee24 100644
--- a/lib/Mojo/Server/Daemon.pm
+++ b/lib/Mojo/Server/Daemon.pm
@@ -54,6 +54,7 @@ sub start {
   elsif (!@{$self->acceptors}) {
     $self->app->server($self);
     $self->_listen($_) for @{$self->listen};
+    $self->ioloop->next_tick(sub { $self->app->server($self, {after_start => 1}) });
   }
 
   return $self;

kraih · 2022-03-04T16:10:32Z

PR seems abandoned. Given our growing review backlog, i'm closing this to make room for other PRs.

kraih added the work in progress label Apr 4, 2021

jhthorsen reviewed Apr 5, 2021

View reviewed changes