search

laravel / octane

Supercharge your Laravel application's performance.
https://laravel.com/docs/octane
MIT License
3.76k stars 293 forks source link

Fixing memory leaks that stem from the cloned application instance #888

Closed AlliBalliBaba closed 4 months ago

AlliBalliBaba commented 4 months ago

This pull request seeks to fix memory leaks like the one in #887. One big downside about Octane's approach of cloning a new application instance on each request is that it's not really possible to track down all references to the original application instance. This can lead to memory leaks that are hard to detect and incompatibilities with other Laravel libraries.

In this pull request I put forward a new approach that seeks to instead keep the same application instance but reset it to a previous state:

octane drawio

This can be achieved pretty elegantly by creating an ApplicationSnapshot that extends the base Application and stores its initial state. Since PHP objects have access to the protected scope of other objects of the same class, the Snapshot is able to reset all Application properties via get_object_vars(). This ends up doing the same thing as clone, but without creating a new instance.

I tested this approach with the Swoole and FrankenPHP runtimes and it indeed fixes the memory leak in #887 and should generally make integrating Octane with other ServiceProviderseasier.

The main changes are in the Worker.php class and the newly added ApplicationSnapshot.php class. All other other changes mainly remove code that becomes redundant through this PR and adjusts tests that expect the application instance to change.

This is just an initial draft. There is probably potential for more code removal and the approach should be thoroughly tested with other first party packages.

github-actions[bot] commented 4 months ago

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

driesvints commented 4 months ago

Marking this as ready for review so it can be reviewed. Thanks!

AlliBalliBaba commented 4 months ago

Something else that would make this even more safe is removing all access to the initial app state in Octane's event listeners. The flushing of scoped instances and so on would then happen right before making the snapshot instead of after each request. That would 100% ensure that only one application instance ever exists and that its state is exactly the same before each request.

Doing so might break custom event listeners people have added to the octane php config though.

taylorotwell commented 4 months ago

This is just an extremely risky PR for us to merge on a patch release. Can we instead focus on just fixing the ViewServiceProvider problem first?

AlliBalliBaba commented 4 months ago

I agree, this should probably be refined more for a minor or major. Fixing the leak might not be possible though without adding an $app setter in the ViewServiceProvider class in laravel/framework. Would the strategy there be to first make a pull request in laravel/framework and then make the pull request here?

driesvints commented 4 months ago

@AlliBalliBaba yes please

AlliBalliBaba commented 4 months ago

I've reset this PR to only specifically fix the #887 memory leak. The PR is waiting for another PR in laravel/framework. It won't break older versions since it checks if the method_exists.

I will revisit the Snapshot approach in a future PR since I think it will make Octane more maintainable in the long run.

nunomaduro commented 4 months ago

Fixed here: https://github.com/laravel/framework/pull/51450.