Working toward Octane support: make core request state worker-safe - #1511
Working toward Octane support: make core request state worker-safe#1511austinderrick wants to merge 6 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edbff29 to
468913a
Compare
…oundary Under PHP-FPM every request gets a fresh process, so state left on a long-lived object never hurts anyone. Under Laravel Octane the same process serves many requests, and anything derived from one request stays visible to the next. Several of these leak one user's data to another. Nothing here requires the companion Storm changes to be present. Winter has to load on a Storm that predates them, and `implements` is resolved when a class is loaded, so naming a contract that a given Storm may not ship makes a class unloadable rather than merely unresettable. The managers therefore declare resetWorkerState() without naming ResetsWorkerState, and the boot-time gates reach runningInApplicationServer() through method_exists(). Where the Storm side is absent, the gates answer exactly as they do today, which is correct: there is no worker support to admit. Winter also takes no dependency on laravel/octane, not even for development. The package appears in suggest only, since a persistent worker is something an application opts into. Every Octane test skips where the package is absent, and the test bootstrap builds its no-op client as an anonymous class so that no file naming an Octane contract has to load when there is nothing to name. Registers Octane's service provider when the package is installed, since Winter replaces Laravel's provider discovery and the provider was never picked up. Adds System\Classes\Octane\ResetsRequestState, which clears request-derived state at the start of every operation rather than the end, because an exception escaping the HTTP kernel skips terminate() and RequestTerminated never fires. Rebuilds the permission-filtered contents of NavigationManager, WidgetManager, SettingsManager, MailManager and ComponentManager per request, while leaving their once-per-worker registrations alone. Clears the per-request caches that PreferenceMaker and the URL maker declare on traits, which needs the using classes resolved because PHP gives each of them its own copy. Keys the preference cache by user. Admits an application server in the boot-time gates of the Backend, System and Cms providers. Those gates test runningInBackend() once per worker, at boot, when no request exists, so back-end registration never ran at all under a worker. Loads a rebuilt CMS code cache file with require rather than require_once. The path is deterministic while the class name inside is random per rebuild, so in a reused process the load did nothing and left the class undeclared, which also disabled the recovery path in handleCorruptCache(). Closes the Twig include gate in a finally block, so a throw during template loading no longer leaves it open for every later request. Invokes the plugin reset on any plugin that implements ResetsWorkerState or simply declares resetWorkerState(), for the same loading reason as the managers above. Tests: 499 green against the companion Storm branch, and 499 green against stock wip/1.3 Storm with laravel/octane absent, where the Octane tests skip. Each fix was verified failing before its change. Also exercised end to end against a Winter based application of about thirty plugins running on Octane and FrankenPHP, whose Cypress suite of 36 groups passes on Octane and PHP-FPM alike.
468913a to
1be38b5
Compare
|
@austinderrick can Octane support be provided by a first party plugin (i.e. Winter.Octane)? |
Hey @LukeTowers : we could move some of this to a first party plugin, like the registerOctane/resetRequestState orchestrator, etc. A big chunk of the changes here are more specific to the Winter internals and how they hold onto variables, so we'll probably need to keep some of them. (And the partner wintercms/storm#241 too). I'll draft something up for it! |
The worker-safety primitives stay in core: resettable manager state, the ResetsWorkerState contract in Storm, and the boot-vs-request gating fixes. What moves to the plugin is the Octane-specific wiring — registering Laravel\Octane\OctaneServiceProvider (Winter disables package discovery), the request-boundary ResetsRequestState listener, and the tests that drive Octane's ApplicationGateway. The plugin declares itself elevated so the reset still attaches when PluginManager::$noInit skips ordinary plugin initialization. The composer suggestion now points at winter/wn-octane-plugin, which requires laravel/octane itself.
austinderrick
changed the title
The plugin's own composer.json requires laravel/octane; core does not need to advertise it.
|
@LukeTowers : I made a repo over here for the plugin and invited you, so we can move it to the Winter org: https://github.com/austinderrick/Winter.Octane Also on this PR and the related Storm one, I rewired them to work with the plugin instead. |
The property is untyped and starts life as null; the docblock claimed it was always a string.
|
@austinderrick if you make me an admin or an owner on the repo then I can transfer it to the Winter organization and you can continue working on it from the official namespace. |
@LukeTowers : Yep! Transfer headed your way. |
|
@austinderrick repository has been transferred and added to packagist: https://packagist.org/packages/winter/wn-octane-plugin |
2 participants
DRAFT UNTIL wintercms/storm#241 is merged!
Summary
Working toward Octane support. Requires wintercms/storm#241 for the primitives it calls.
Under PHP-FPM every request gets a fresh process, so state left on a long-lived object never hurts anyone. Under Laravel Octane the same process serves many requests, and anything derived from one request stays visible to the next. Most of what follows was found by running a real application on a worker and watching it misbehave, not by reading the source. Several of these leak one user's data to another.
None of this changes PHP-FPM behavior. What remains in this PR is inert without a worker: it makes core's request-derived state resettable, and nothing in core resets it.
Split with the Winter.Octane plugin
Following the suggestion in review, the Octane-specific integration now lives in a first-party plugin: Winter.Octane. The plugin registers Octane's service provider (Winter disables Laravel's package discovery, so an installed
laravel/octaneotherwise does nothing), clears request state at the start of every operation ahead of Octane's own listeners, callsresetWorkerState()on any plugin that defines it, and carries the worker test suite, which drives Octane's realApplicationGatewayagainst a single application instance. Itscomposer.jsonrequireslaravel/octane, so this repo no longer suggests the package.Two consequences of plugin placement are worth weighing in review:
elevated, so it still loads on the privileged routes and commands wherePluginManager::$noInitskips normal plugin initialization. Without that, the first request a worker served could decide that the reset never attaches for the worker's lifetime.What stays in this PR are the changes a plugin cannot make: core classes exposing their request-derived state in resettable form, and boot-time decisions that must admit a worker.
Manager state rebuilt per request
Five managers build their contents once and cache the result. The current user's permissions filter some of that content. A copy built for one back-end user then went to whoever came next.
Backend\Classes\NavigationManagercaches the main menu and side menus after permission filtering.Backend\Classes\WidgetManagercaches the report widget list.System\Classes\SettingsManagercaches settings items after filtering.System\Classes\MailManagercaches registered mail partials and layouts.Cms\Classes\ComponentManagercaches resolved component details.Each now separates what is registered once per worker from what is derived per request, and rebuilds only the second part. Registrations survive, which matters because rebuilding them on every request would cost more than the leak.
Trait declared caches
Backend\Traits\PreferenceMakerand the URL maker keep per-request caches in static properties declared on the trait rather than on the classes that use it.PHP gives every using class its own copy of a trait's statics, so assigning through the trait clears nothing. The reset resolves the classes that use each trait and clears the property on each one. The preference cache is also keyed by user, since the same key previously served whichever user asked first.
Boot time gates
Backend,SystemandCmsservice providers registered parts of themselves only whenrunningInBackend()was true. Winter evaluates that test once per worker, at boot, when no request exists. Back-end registration therefore never ran at all under Octane.The gates now also admit an application server, using the detection method added in wintercms/storm#241.
CMS code cache
Cms\Classes\CodeParser::rebuild()wrote a generated class to a deterministic path and loaded it withrequire_once. The class name inside is random for every rebuild, andrequire_oncekeys on the path.In a process serving more than one request, the path was normally included already. The load then did nothing at all and left the new class undeclared:
This also disabled the recovery path in
handleCorruptCache(), whose whole purpose is to rebuild after finding the cache unusable. It rebuilt, then could not load what it had rebuilt. Usingrequirecannot redeclare anything, because the class name is unique to each rebuild.Twig include gate
System\Twig\Engineopened a gate before loading a template and closed it afterward. A throw during loading skipped the close, so the gate stayed open for every later request the worker served.The close now happens in a
finallyblock.Testing
Remaining in this PR:
modules/cms/tests/classes/CodeParserWorkerTest.phpmodules/system/tests/twig/EngineTest.phpThe worker tests moved to Winter.Octane, where they exercise the plugin's own wiring rather than core's: request state isolation, listener ordering and priority, cross-user manager state, the reset manifest, and plugin reset discovery, all dispatched through Octane's real gateway.