laravel / octane

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

Memory Leak in ViewServiceProvider #887

Closed AlliBalliBaba closed 1 month ago

AlliBalliBaba commented 2 months ago

Octane Version

2.3.10

Laravel Version

11.4.0

PHP Version

8.3.7

What server type are you using?

FrankenPHP

Server Version

1.1.4

Database Driver & Version

No response

Description

While doing some tests with Laravel Octane and the FrankenPHP/Swoole runtimes I noticed that the application was getting slower as time went on, especially when setting a big number for max-requests. After digging around I found that it only happens when a blade file is rendered. I found out the ViewServiceProvider is the issue since it's holding a stale version of the $app container.

More specifically it's line 166-168 where a terminating callback gets added to the app every time a new Blade compiler is resolved (see image).

bug

For every request a new callback gets added to the original app instance. At some point there are thousands of callbacks in the terminatingCallbacks[] array of the original $app , which slows down the application and is a memory leak.

It suspect Octane is only giving the new $appinstance to the ViewFactory but not to the ViewServiceProvider

Steps To Reproduce

Set up a clean Laravel installation with Laravel Octane. install dependencies with composer install install any runtime with php artisan octane:install, I'll choose FrankenPHP

Go into the vendor folder of your application and modify the Illuminate\Foundation\Application like this to dump out the amount of registered callbacks:

public function terminating($callback)
    {
         $this->terminatingCallbacks[] = $callback;
+       Log::info(count($this->terminatingCallbacks) . " callbacks registered");

        return $this;
    }

start octane with 1 worker for simplicity: php artisan octane:start --server=frankenphp --host=yourhost --port=80 --max-requests=1000' --workers=1 --admin-port=2019

visit the default home page /home in the browser multiple times. In your laravel.log you can now see that the number of callbacks increases on each request.

driesvints commented 2 months ago

Thanks for reporting this @AlliBalliBaba. Would appreciate a PR if anyone can figure one out.

github-actions[bot] commented 2 months ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

AlliBalliBaba commented 2 months ago

I think the main issue here and with possible other memory leaks is that Octane changes the reference to the application container on each request and then attempts to update all references to the old instance. A safer approach would be to not clone the container, but reset it to an earlier state instead.

Here is what I mean:

octane drawio

I think this could be done rather easily because of how PHP properties can be accessed based on class scopes:

class ApplicationSnapshot extends Application
{

    public function loadStateInto(Application $app): void
    {
        foreach (get_object_vars($this) as $key => $value) {
            $app->$key = $value;
        }
    }
}

Not changing the app reference makes Octane more resilient towards memory leaks and overall more compatible with 3rd party libraries . This wouldn't require too much refactoring and makes a lot of the GiveNewApplicationInstanceToX listeners redundant.

I could draft a PR but want to know first if the idea won't get outright rejected.

AlliBalliBaba commented 2 months ago

Since I'm sick at home today anyways, I created an initial draft. It would be nice if someone could look over it.

driesvints commented 1 month ago

@AlliBalliBaba get well soon đź‘Ť

AlliBalliBaba commented 1 month ago

I can confirm that this leak is fixed in the newest Laravel Version 11.8.0. Closing this for now