Do not enable preloading by default

[dentarg]

I think we should revert this code from #2143

puma/lib/puma/configuration.rb

Lines 149 to 151 in 7aa62c7

	if !@options[:prune_bundler]
	default_options[:preload_app] = (@options[:workers] > 1) && Puma.forkable?
	end

The idea for Puma 5 was to make preload_app! the default, but it is only true if you use WEB_CONCURRENCY to configure the number of workers (because this code runs before we have loaded the config file).

We can of course fix this, but I'm not so sure it is a good idea. From the issues, it seems like using phased restarts is popular, and enabling preload_app! breaks phased restarts (Puma restarts normally instead). It would also make the defaults a bit strange, out of the box, phased restarts would only be available when using 1 worker (#2481).

[cjlarose]

I think rolling back the code might be the right call or at least we need to make this very clear in the 5.x upgrade docs (like you already proposed in #2481).

The change in #2143 was probably meant to be basically backwards-compatible with puma 4.x: if your application was using phased restarts, then you already had prune_bundler on, so the new default should have no effect for you. But I think the logic of that PR was that if you didn't already have prune_bundler, on, then there's no reason to not have preload_app! on, since it just saves memory and you're not doing phased restarts anyway.

I think that logic is based on the false premise that if you don't have prune_bundler on, you're not using phased restarts. While it's true that under a configuration where prune_bundler is off, you wouldn't be able to change the versions of the gems that your application is using, there are still reasons to perform phased restarts, for example, if your application suffers from memory leaks and you just want fresh workers every so often. If you were a user that used phased restarts like that, you would have been bitten unexpectedly by your first phased restart after upgrading to puma 5.x.

[nateberkopec]

I think that logic is based on the false premise that if you don't have prune_bundler on, you're not using phased restarts.

I agree. One additional thing here is that you can't figure out if you configured Puma correctly to allow phased restart until you actually try to phased restart it.

Possible ideas:

1. Log on startup what signals/restarts/features are available for post-startup fiddling
2. A new configuration option which signals that you will try to phased_restart the process in the future. The initial version would probably only just blow up if you also have preload_app on + not try to enable preload_app by default.

Now that this is shipped on master, a simple revert will also break anyone who is depending on the new functionality to cause preload_app!. As Chris mentioned, this behavior breakage only affects a subset of phased_restart users.

[nateberkopec]

I think we should make phased restart users "opt-in" to a configuration option that signals they want to be sure, before boot, that their processes will respond correctly to a phased-restart signal. We thought prune_bundler was that opt-in, but it isn't. That's the bug here.

The 5.0 change implicitly turned off phased-restart by default. We should document that, and then add a new DSL option, phased_restart! or something, that checks for phased restart compatibility, blows up if some irreoncilable problems occur (e.g. user has preload_app! in their puma.rb too).

Maybe in the future this option can do some smart checking/recommendation to tell the user if they need prune_bundler.

[nateberkopec]

Also I think preload_app! being set only if WEB_CONCURRENCY was set was a mistake. It should be on by default if using cluster mode by any means.

[dentarg]

Okay, so we still want preload_app! to be the default in Puma 5

Should we make the signal for phased-restart (USR1) a NO-OP when preload_app! enabled? Is that what you mean with your phased_restart! suggestion @nateberkopec?

I'm raising this discussion about going forward with preload_app! as the default, because we thought we made it default, but that was only true when using WEB_CONCURRENCY. So now Puma 5 has been out for two months and a few releases have been made. If we fix the bug, and make preload_app! default for real, will it cause problems that people doesn't expect when they update Puma with just a patch (or minor) release?

[nateberkopec]

If we fix the bug, and make preload_app! default for real, will it cause problems that people doesn't expect when they update Puma with just a patch (or minor) release?

Oh definitely not, I could've been more clear here: this would be done for Puma 6.

Is that what you mean with your phased_restart! suggestion @nateberkopec?

Hmmm... no. I'll make a PR.

[nateberkopec]

Actually, now that I think about it, I think what I'm really proposing is to keep things as-is but document/encourage usage of preload_app! false, as that's all any phased_restart! DSL option would actually do.

Once Puma 6 rolls around, make preload_app! default for everyone, document that you must use preload_app! false if you want to use phased restarts. Not a difficult upgrade process IMO.

[cjlarose]

it seems like using phased restarts is popular, and enabling preload_app! breaks phased restarts (Puma restarts normally instead)

One thing that is kinda awkward about phased restarts as they're implemented right now is that phased restarts are not inherently incompatible with preload_app, but puma cluster code just refuses to perform a phased restart if preload_app is on. I think that is because it assumes incorrectly that if you're using preload_app, there's no reason to perform a phased restart.

puma/lib/puma/cluster.rb

Lines 203 to 204 in 5af91ff

	def phased_restart
	return false if @options[:preload_app]

If we instead just changed that behavior in cluster.rb to allow the user to perform a phased restart, even though preload_app is on, then I think that's a way to make the upgrade to puma 5 more seamless, without rolling back the change to preload the application by default.

More concretely, if you were a user that wasn't explicitly opting-in to preload_app in puma 4.x and you were using phased restarts (not to upgrade your application, but just to get fresh worker processes), then the only difference you would experience after upgrading to 5.x is less memory usage thanks to COW, in the spirit of the intended changes by #2143 . Phased restarts would continuing to work for you in 5.x. This is better than the current 5.x upgrade experience for those users since right now they're getting unexpected hot restarts.

If you were a user that was opting into preload_app in 4.x, you probably weren't trying to upgrade your application with phased restarts. If you did try to issue a phased restart in 4.x, then puma would just perform a hot restart. In puma 5.x, you will now instead get a true phased restart, but shouldn't be a problem for anyone.

The only problem would be that if you were a user that did opt into preload_app app 4.x, then changed the version of the application, then issued a phased restart, in puma 4.x, the cluster would gracefully fall back to a hot restart. In puma 5.x with the change I propose, the cluster would perform a phased restart, but the application would not actually be upgraded (the new workers would be using the old preloaded version of the application). This is a bad experience, but IMO there shouldn't be too many of these users anyway and this is the use case we can draw attention to in the 5.x upgrade docs.

If we made this change in puma 5.x now, it would mean that we can keep the preload-by-default behavior (and perhaps more importantly, we can confidently fix it so that it works when setting workers explicitly, not just when using WEB_CONCURRENCY). We'd have to update the upgrade docs to explain the one case where 4.x users would experience a difference in behavior: if you were previously initiating phased restarts and had preload_app on and relied on the fallback behavior to hot restarts, that no longer works. Those uses will have to disable preload_app and probably turn on prune_bundler if they want to upgrade their application.

[cjlarose]

I should say, too, that I don't know if I'm 100% an advocate of the solution I proposed in the previous post. Just wanted to present it as an option.

So far it seems that these are these options

1. Do nothing. Allow the preload-by-default behavior if WEB_CONCURRENCY is used to configure the number of workers, but do not introduce a change to "fix" this by preloading-by-default regardless of how the user enables puma cluster mode. Make the difference in behavior clear from the puma 4.x behavior in the upgrade docs (we've already done this).
2. "Fix" the bug that makes it so that preload-by-default only works when WEB_CONCURRENCY is used to specify the number of workers. This could make it so that the application is unexpectedly preloaded, breaking phased restarts, so this change probably warrants a major version bump.
3. Revert preload-by-default behavior in a minor puma release. It's not 100% clear if this would be considered a "breaking change" from semver perspective, but I could see users being surprised if they're upgrading from a previous 5.x release. Users probably wouldn't expect to have to update their configuration files as part of a minor release, but they might have to if they were relying on the preload-by-default behavior.
4. Revert preload-by-default behavior, but make it a new major release (6.x). It would be a clear signal to all users that they might have to make updates to their configuration files if they want the behavior they were relying on in previous puma releases. But of course there's a real human cost associated with a major release and doing major releases so close together can be understandably fatiguing.
5. Allow phased restarts on the puma cluster, even if preload_app is on (either because the user chose to preload the app explicitly, or because it was enabled for them by default). The pros/cons of this approach are in the previous comment. We might be comfortable making this change in a minor release, but it's not 100% clear.

If we do decide to do a puma 6.x, we can take the opportunity to carefully consider all of our options we have with regard to what the best configuration experience would be for our users.

[nateberkopec]

I think we fix this by making clear what we didn't in the Puma 5.0 upgrade docs: to get phased restarts, you must preload_app! false or prune_bundler.

The bottom line here is that a significant portion of the userbase runs cluster w/o needing phased restarts. Those people should get opted-in to preload_app. If you want phased restarts, you need to opt in to that instead. We changed who needs to opt in to what incompletely in Puma 5, and that's my mistake. It should have been a complete shift.

We already log whether or not phased restart is available on boot, but I think we could make it clearer by logging slightly differently:

[13064] Puma starting in cluster mode...
[13064] * Version 5.0.4 (ruby 2.7.2-p137), codename: Spoony Bard
[13064] * Min threads: 0, max threads: 5
[13064] * Environment: development
[13064] * Process workers: 2
[13064] * Restart types available: hot, phased
[13064] * Listening on http://0.0.0.0:9292
[13064] Use Ctrl-C to stop
[13064] - Worker 0 (pid: 13066) booted, phase: 0
[13064] - Worker 1 (pid: 13067) booted, phase: 0

Part of the UX problem here is that if phased restart is NOT available, we don't say anything. In this new logging I propose, you would see:

* Restart types available: hot

Maybe we can do something even cleverer with emoji or something to make it even clearer:

* Restart types available: ✅ hot ❌ phased

[nateberkopec]

I like the emoji log a lot but I want to be sure there aren't any major issues/incompatibilities with logging emoji before I decide to commit that.

[nateberkopec]

I asked on Twitter and apparently some people don't like emoji in their logs. So maybe more like:

* Restart types available: (✔) hot (✖) phased

Dingbats, yay!