Breaking changes in 9.0

[kraih]

We will soon be releasing Mojolicious 9.0. That means we will have the rare opportunity to make significant breaking changes. With #1266, #1660 and #1369 there are three changes already planned. But that doesn't mean we have to stop there. If you have more ideas please post them in this thread.

Update: 9.0 has been released. See you all again for 10.0! 🎉

[kraih]

Mentioned on IRC, we could finally remove auto rendering.

Result: Only partially done, very controversial.

[Grinnz]

My concern is that lite apps tend to be heavily dependent on route definitions like get '/foo'; and this is a big selling point for ease of starting out, as @s1037989 mentioned. Even with a core plugin that changes that, the plugin would affect all routes and not just those simple ones.

[marcusramberg]

I'm 👎 on removing autorendering as a feature. If there's anything we can do to make it more obvious that it's happening I would like that tho.

[kraih]

I've decided to implement this partially after all. Some cases that are pretty much guaranteed to be user mistakes are now exceptions thrown by the router. Specifically if you route to a controller, that controller class needs to exist. So you cannot do any '/foo' => {controller => 'foo', action => 'bar'}; anymore and have it render templates/foo/bar.html.ep if the controller class doesn't exist.

Actions that do not exist, but where the controller class does, will still trigger auto rendering.

[s1037989]

And any '/foo'; will still render templates/foo.html.ep?

I'm happy to have the "Class is not a controller" message go away. I don't know why, but those were always tricky to identify and debug.

[kraih]

And another small change. If auto rendering fails it now results in a 500 instead of 404 response. 11b2772

[kraih]

We could remove Mojo::IOLoop::Delay, which has become kinda obsolete with the addition of Mojo::Promise.

Result: Done.

[kraih]

Without Mojo::IOLoop::Delay we could also remove the ioloop attribute from Mojo::Promise. Making it only use Mojo::IOLoop->singleton. That would fix the problem where Mojo::Promise::all and friends have to rely on the first promise passed to them containing the Mojo::IOLoop instance used for the promise they return.

[Grinnz]

For the record I am in favor of spinning out Mojo::IOLoop::Delay, but I am not in favor of removing the ioloop attribute (of course I can be outvoted). This would prevent the ability to use promises in a blocking fashion while the singleton loop is running; most commonly waiting for multiple events before continuing, coordinated with all.

[Grinnz]

Prior discussion: #1193, #1275, #1505

[kraih]

Mojo::IOLoop::Delay is now gone from the 9.0 branch. a1c30f6

[kraih]

Removing the ioloop attribute from Mojo::Promise did turn out much harder, since many of our tests depend on it. So i will give up on that for now.

[jberger]

Per discussion on IRC, it is annoying to have to do map { $_->[0] } on the result of Mojo::Promise::all every time, simply because the Mojo::Promise api allows for returning multiple results. In practice the number of times I've seen a promise return multiple results is vanishingly small.

The proposal is to (a) make Mojo::Promises resolve with only a single value. Obviously a particular promise could return as many results as it would want by returning an array reference. In practice this would be a small breaking change to Mojo::Promise as, again, I don't think too many people return multiple values.

This would then allow (b) Mojo::Promise::all to not need to return an array reference for each input promise but rather just the value, since there can only be one. This would be a major breaking change to all however the fix is probably rather trivial and it should fail hard in most cases (dereferencing a non-reference).

Result: Probably not happening, too much work.

[kraih]

Maybe even worse is that all_settled would still support multiple values, while all would not. That's just wrong.

[jberger]

Echoing a discussion from IRC, to clarify, my intention was to make everything single-valued for consistency. Point (b) shouldn't have focused exclusively on all but rather all of the multi-promise collectors

[Grinnz]

Despite my hesitance with how much will have to be updated for this, I am still 👍 as long as the whole API is changed consistently.

[kraih]

Ok, i'll clarify my vote again. I'm probably 👍 if everything gets change consistently to use a single value, and 👎 if the proposal only ends up with Mojo::Promise::all being changed. But i do still have reservations since there is a lot of unknowns.

[marcusramberg]

I have the same position as @kraih , if we change this we should change it for resolve and then as well as all. I guess it's possible to give a pretty good hint to developers about what has changed if they pass multiple values?

[s1037989]

Referring to #1660, can Mojo::Log behave like sprintf?

$c->log->info('you just got trolled by visitor number %09d', $visitor);

Result: Probably not happening, impossible.

[jhthorsen]

This reminds me that I should convert https://metacpan.org/pod/Mojolicious::Plugin::Logf into Mojo::Log::Role::Logf

[kraih]

Doesn't work, multiple args already have meaning.

[kraih]

A possible change we've been talking a few times before is to make Mojo::DOM::all_text ignore <script> and <style> blocks.

Result: Done.

[kraih]

Think i'd be in favour of that.

[Grinnz]

Previous attempt: #1502

[kraih]

I've fixed the previous attempt, so this is done now.

[kraih]

We could also finally support deployment specific plugins. Those would be declared in the application config file (probably with the plugins or extra_plugins key), by a user, without modifying the code. Mojolicious::Plugin::SetUserGroup for example would be one such plugin.

Result: Done, but might still need small changes.

[Grinnz]

+1. The key should be an array in case ordering of plugin loading is important. The part that still needs to be hashed out is when such plugins would be loaded, relative to the startup method.

[kraih]

I've thought about when it should be loaded, and the only solution i can think of is at the same time the Config plugin is loaded. So the plugin loading would happen inside the Config plugin (and its JSON/YAML subclasses).

[kraih]

So, yes, it would basically be a Mojolicious::Plugin::Config feature.

[s1037989]

Oh wonderful! This would solve #1427!

[kraih]

The feature now exists in the branch. Some details might still need to be adjusted though, like the name and data structure used. e06ed19

[kraih]

Mojo::UserAgent::local_address has always been a bit awkward. But at the time it was added we were still supporting various IO::Socket::* classes. Now it's just IO::Socket::IP, and that means we could very easily use a more generic attribute like Mojo::UserAgent::socket_options now.

$ua->socket_options->{LocalAddr} = '127.0.0.1';

Result: Done, but with deprecations in an 8.x release.

[drclaw1394]

Hi, I'm a Mojolicious novice but I thought I would give my suggestion anyway.

Any chance or reducing memory footprint? I've been looking at using the Mojolicious websocket support in an embedded environment with limited RAM. 40MB+ minimum size for single process with no application code running is just too high for my case. Despite my use case, reduced memory I assume would be better for performance in general.

The Mojo::Base class uses the Mojo::Util package, which itself uses a whole bunch of modules and memory. For example loading Pod::Usage and Data::Dumper modules (as Mojo::Util does) adds over 6MB and 2.5MB of memory, before the Mojo::Base class has 'done anything'

Also the List::Util while a small module, is only used a single instance of the min sub (in the unindent sub). Perhaps could roll your own to save on a dependency?

Would it be viable to move some of the features of Mojo::Util to a more specific, restricted scope module(s)?
For example the afore mentioned Pod::Usage and Data::Dumper could be in some sort of 'debug' module the user can optionally use.

Does the base class have to depend on the Mojo::Util at all? It's not really a utility module if its not optional.

Just my two cents worth. Thanks for your time.

Result: Probably not happening, controversial.

[kraih]

If there were real low hanging fruits, like removing List::Util would gain us like 10MB, then we'd probably do it. But that's just not the case. Trying to reduce memory usage seriously would come at the cost of performance and maintainability.

[jberger]

We also have to think about preforking. While a minimal app might want to lazy load as much as possible, preforking servers (which are probably are must important target) want to load as much as possible before forking to take advantage of CoW semantics. Obviously there's going to be a balance but if push came to shove I'd have to favor preforking servers over resource-constrained scripts.

[Grinnz]

I'd also add that this sort of refactoring can be done at any time, and does not need to be related to a major version.

[kraih]

It would definitely be easier to split up Mojo::Util in a major release, if we were to ever consider it.

[jberger]

I don't see any need to split it up.

It's not really a utility module if its not optional.

It isn't "optional utilities" it is "one-off functions (read: not classes) that don't have someplace else to live".

[kraih]

Should we disallow special stash values like :controller and :action in route patterns? This is a pretty big security risk, and we currently require special features that cost us performance just to mitigate this risk. At the same time the feature has been discouraged for years, and is only mentioned once in the guides, with a warning recommending not to use the feature.

Result: Done.

[kraih]

I propose that we make patterns containing reserved stash values as patterns die during application startup. That would also allow us to remove the whole hidden method system, making the actual dispatching faster.

[jberger]

It would also allow can with helpers IIRC

[kraih]

And it's implemented. 1261e52

[kraih]

The special handling of can is still needed for auto rendering though.

[kraih]

Make $c->render raise an exception if nothing could be rendered, instead of generating a 404 response (which never made any sense). We already have $c->render_maybe to handle cases where a template might not exist. The current behaviour just hides bugs.

Result: Done.

[kraih]

And implemented. e0d3b42

[kraih]

That still leaves auto rendering with sometimes resulting in 404 responses. But i don't think there is much we can do there without killing the feature.

[s1037989]

What about adding an at() method -- like timer() and recurring() -- to Mojo::IOLoop so that an action can be done at a specific time of day?

Mojo::IOLoop->at('1p' => sub { shift->app->log->info('Runs every day at 1p') });
Mojo::IOLoop->at('Tue 1p' => sub { shift->app->log->info('Runs every Tuesday at 1p') });
Mojo::IOLoop->at('Tue' => sub { shift->app->log->info('Runs every Tuesday at midnight') });
Mojo::IOLoop->at('first monday 1p' => sub { shift->app->log->info('Runs every first day of the month at 1p') });
Mojo::IOLoop->at('February 6, 2022 1p' => sub { shift->app->log->info('Runs February 6, 2022 at 1p') });

I feel like this would be a really handy feature, but I can also see it being superfluous as timer and recurring are more than capable of meeting the needs of at, just not at specific times of day or days of week.

The value to me would be mostly around sending notifications, like a day's report. You wouldn't want a report delivered x hours from whenever the server happened to restart, you'd want your report at the same time each day.

Another example: how would one build a calendaring app in Mojolicious and be able to send reminder notifications? When a calendar event for Feb 6 at 1p is created, the app could calculate 24 hours before, or 7p the day before in order to know when to send the reminder, but how would the notification actually get sent at that time?

Something similar in Minion, alternatively or additionally, would also be great for the same reason.

Result: Probably not happening, does not require any breaking changes.

[Grinnz]

The main thing that makes this complicated is time zones. It also doesn't really have any benefit from being done in core instead of as a wrapper.

[kraih]

Pretending to have a cron built-in seems rather counter productive to me. We really don't have that. Timers are ephemeral, process/event loop specific and pretty imprecise. It's a completely different use case. People will just end up with timers firing at random times in more processes than they anticipated.

[kraih]

Something related that could be useful however would be a simpler way to specify relative times like Mojo::IOLoop->timer('10m' => sub {...}) (10 minute timers). That could be a Mojo::Util function for us to use in timers, cookies, caches and so on.

Edit: Actually, i think i changed my mind. Once we start doing this it's hard to draw the line, there would probably be demand for similar functions to handle byte values and the like too.

[Grinnz]

People can use https://perldoc.perl.org/Time::Seconds to simplify times like that a bit.

[kraih]

All of this also doesn't require any breaking changes, so there is no need to have it in 9.0.

[s1037989]

Would it be possible for the managing Mojo::Server process to manage an additional and separate pool of processes with exactly one process -- an assistant manager, if you will?

$ perl assistant prefork
[2021-02-07 10:39:50.21541] [3284802] [info] Manager 3284802 started Assistant 3284809
[2021-02-07 10:39:50.21544] [3284809] [info] I am Assistant 3284809 to Manager 3284802
[2021-02-07 10:39:50.21803] [3284802] [info] Listening at "http://*:3000"
Web application available at http://127.0.0.1:3000
[2021-02-07 10:39:50.21814] [3284802] [info] Manager 3284802 started
[2021-02-07 10:39:50.21969] [3284810] [info] Worker 3284810 started
[2021-02-07 10:39:50.22007] [3284811] [info] Worker 3284811 started
[2021-02-07 10:39:50.22069] [3284812] [info] Worker 3284812 started
[2021-02-07 10:39:50.22128] [3284802] [info] Creating process id file "/tmp/prefork.pid"
[2021-02-07 10:39:50.22128] [3284813] [info] Worker 3284813 started
[2021-02-07 10:39:51.21657] [3284809] [info] Manager 3284802 Assistant 3284809: Sun Feb  7 10:39:51 2021
[2021-02-07 10:39:52.21719] [3284809] [info] Manager 3284802 Assistant 3284809: Sun Feb  7 10:39:52 2021
[2021-02-07 10:39:53.21771] [3284809] [info] Manager 3284802 Assistant 3284809: Sun Feb  7 10:39:53 2021
^C[2021-02-07 10:39:53.37036] [3284802] [warn] Stopping worker 3284813 immediately
[2021-02-07 10:39:53.37055] [3284802] [warn] Stopping worker 3284810 immediately
[2021-02-07 10:39:53.37065] [3284802] [warn] Stopping worker 3284812 immediately
[2021-02-07 10:39:53.37073] [3284802] [warn] Stopping worker 3284811 immediately
[2021-02-07 10:39:53.37271] [3284802] [info] Worker 3284812 stopped
[2021-02-07 10:39:53.37285] [3284802] [info] Worker 3284813 stopped
[2021-02-07 10:39:53.37299] [3284802] [info] Worker 3284810 stopped
[2021-02-07 10:39:53.37313] [3284802] [info] Worker 3284811 stopped
[2021-02-07 10:39:53.37332] [3284802] [info] Manager 3284802 stopped
[2021-02-07 10:39:54.21858] [3284809] [info] Manager 3284802 Assistant 3284809: Sun Feb  7 10:39:54 2021
[2021-02-07 10:39:54.21874] [3284809] [info] Manager 3284802 Assistant 3284809: Good bye

$ cat assistant 
use Mojolicious::Lite -signatures;
use Mojo::IOLoop;

my $manager = $$;
app->hook(before_server_start => sub ($server, $app) {
  if (my $assistant = fork) {
    # parent
    app->log->info(sprintf 'Manager %d started Assistant %d', $manager, $assistant);
  }
  else {
    # child
    my $assistant = $$;
    app->log->info(sprintf 'I am Assistant %d to Manager %d', $assistant, $manager);
    my $ioloop = Mojo::IOLoop->new;
    $ioloop->recurring(1 => sub { app->log->info(sprintf 'Manager %d Assistant %d: %s', $manager, $assistant, scalar localtime); $ioloop->stop unless kill 0, $manager });
    $ioloop->start unless $ioloop->is_running;
    app->log->info(sprintf 'Manager %d Assistant %d: Good bye', $manager, $assistant);
    exit;
  }
});

app->start;

Result: Probably not happening, does not require any breaking changes and is a planned feature for a future minor release already.

[kraih]

What breaking changes would that require?

[kraih]

And in case you're not following the IRC channel, this is already a planned feature for some future minor release (aiming at supporting Minion workers to be started together with daemon and prefork). Nobody has volunteered yet to find solutions for all the problems that entails though.

[kraih]

I've opened #1694 as a reminder.