barryvdh / laravel-debugbar

Debugbar for Laravel (Integrates PHP Debug Bar)
MIT License
17.4k stars 1.56k forks source link

Memory Leaks in Octane #1172

Closed kiroto123 closed 3 years ago

kiroto123 commented 3 years ago

Hi

Those listeners creates a memory leak in Octane

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L335

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L379

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L386

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L393

laravel/octane#243

barryvdh commented 3 years ago

Does this also mean you get duplicated queries etc in Debugbar after the first run?

kiroto123 commented 3 years ago

No.

The sandbox instance share the same eventManager with app instance, and with the request coming, debugbar add listener to eventManager, but does not destroy after the request. The listener's num keep going.

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L335-L367

In those code. first, we got a db listener

https://github.com/barryvdh/laravel-debugbar/blob/6420113d90bb746423fa70b9940e9e7c26ebc121/src/LaravelDebugbar.php#L345

$this pointer points to current debugbar instance, assume it's id is 33.

Then, another request coming, add a new db listener, and $this pointer points to new debugbar instance, assume it's id is 55.

Now we have two db listeners, one points to 33 and other points 55.

Memory leaks happen.

Also, if \Illuminate\Database\Events\QueryExecuted event triggered, it will executed twice, and the 33 debugbar instance points to an old app instance, It will cause an error like

Illuminate\Contracts\Container\BindingResolutionException

Target class [config] does not exist.

You can add code

<?php

namespace App\Octane;

use Laravel\Octane\Events\RequestTerminated;

class ListenersInManager
{
    public function handle($event)
    {
        if (! $event instanceof RequestTerminated) {
            return;
        }
        $app     = $event->app;
        $sandbox = $event->sandbox;
        $eventManager = $app->make('events');
        $eventManagerInSandbox = $sandbox->make('events');
        $this->Print('eventManager', $eventManager);
        $this->Print('eventManagerInSandbox', $eventManagerInSandbox);
    }

    protected function Print($name, $eventManager)
    {
        echo sprintf("%s event manager id: %d\n",$name, spl_object_id($eventManager));
        $ref = new \ReflectionObject($eventManager);
        $listenersProperty = $ref->getProperty('listeners');
        $listenersProperty->setAccessible(true);
        $listeners = $listenersProperty->getValue($eventManager);
        $buf = [];
        foreach ($listeners as $key => $listener) {
            $buf[$key] = is_array($listeners) ? count($listener) : 1;
        }
        var_export($buf);
        echo PHP_EOL;
    }
}

and add it to octane config file listeners config.

        OperationTerminated::class => [
            FlushTemporaryContainerInstances::class,
            \App\Octane\ListenersInManager::class,
            // DisconnectFromDatabases::class,
            // CollectGarbage::class,
        ],

Start server with command php artisan octane:start --workers=1 --task-workers=1 and request

You will see events

listeners' num are keep going

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this issue is still present on the latest version of this library on supported Laravel versions, please let us know by replying to this issue so we can investigate further. Thank you for your contribution! Apologies for any delayed response on our side.

barryvdh commented 3 years ago

Keep it open please.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this issue is still present on the latest version of this library on supported Laravel versions, please let us know by replying to this issue so we can investigate further. Thank you for your contribution! Apologies for any delayed response on our side.