Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions Renderer Listener causing memory leak #52416

Closed
amcsi opened this issue Aug 7, 2024 · 6 comments
Closed

Exceptions Renderer Listener causing memory leak #52416

amcsi opened this issue Aug 7, 2024 · 6 comments

Comments

@amcsi
Copy link

amcsi commented Aug 7, 2024

Laravel Version

11.20.0

PHP Version

8.3.10

Database Driver & Version

No response

Description

The linked file from the linked PR is causing a memory leak: https://github.com/laravel/framework/pull/51261/files#diff-dd48a4b628c66b2c2388be3d8f6c63911e5cef6b1bfb483fe8c36739087ce40f (#51261)

Why?
This listener class implicitly listens to query execution events, and then - despite me not opting into logging queries - this class stores executed queries in it, though limited to 100.

This is bad when doing many bulk updates in an (ETL) loop, because it stores large bulk insert SQL queries in memory, causing high memory usage.

OK, to be fair, this only happens if Debug mode is on. But I did spend a bunch of time debugging why memory usage was high while developing an ETL tool locally, because I didn't know there was a recent code change that kept query logs that I didn't opt into.

Steps To Reproduce

foreach ($lazyCollectionWith100000Values->chunk(1000) as $rows) {
  \DB::table('some_table')->insert($rows->toArray());
}
// 100 x 1000 rows worth of values are stuck in memory.
@hugoboss17
Copy link

As it's mention here
you can disable the logging and enable it after the bulk insert.

this will result in reduced memory usage and not throw the exception.

@amcsi
Copy link
Author

amcsi commented Aug 7, 2024

Yes, assuming you didn't create any event listeners that you did actually want to use

@hugoboss17
Copy link

hugoboss17 commented Aug 7, 2024

If you still need to have event listeners on this exact queries you can do the setEventDispatcher referencing which event listeners to have.
Pretty much excluding the one you're getting the exception and including your desired one.

https://laravel.com/api/11.x/Illuminate/Database/Connection.html#method_setEventDispatcher

But I may be seeing this wrong :)

@driesvints
Copy link
Member

Hi @amcsi. Since you mention this only happening with debug mode I don't consider this a huge issue. @hugoboss17 also gives a workaround (thanks!).

@amcsi
Copy link
Author

amcsi commented Aug 9, 2024

The workaround isn't perfect, because maybe one would want to listen to events in a custom listener. But at least I wanted to make sure there was an Issue for this so that if in the future anyone would get confused about high memory usage locally or on a dev server, they would save time by finding this Issue by googling.

@amcsi
Copy link
Author

amcsi commented Aug 29, 2024

So for projects where you don't need to listen to any DB events at all, this is the workaround in the boot() method of any service provider:

\DB::connection()->setEventDispatcher(new \Illuminate\Events\Dispatcher());

This is better than \DB::connection()->unsetEventDispatcher();, because otherwise some Feature tests would fail.


In the event that you do want to listen to other DB events, you can create a new override class for the high memory usage listener class:

<?php

declare(strict_types=1);

namespace App\Exceptions;

use Illuminate\Database\Events\QueryExecuted;
use Illuminate\Foundation\Exceptions\Renderer\Listener;

/**
 * Override of Laravel's exceptions listener.
 */
class ExceptionsListenerOverride extends Listener
{
    public function onQueryExecuted(QueryExecuted $event)
    {
        // (...Or you could have a condition here to not return, but rather call the parent, if you do want the log-to-memory behavior for the exceptions HTML page.)
        
        return;
    }
}

...And then use that overridden class this way in any service provider's register() method:

$this->app->bind(Listener::class, ExceptionsListenerOverride::class);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants