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

[6.x] Ensure Application::$terminatingCallbacks are reset on Application::flush() #31413

Merged

Conversation

@markokeeffe
Copy link
Contributor

markokeeffe commented Feb 10, 2020

I recently registered a terminating callback in the constructor of one of my services e.g.

$this->container->terminating(function () {
    $this->doSomething();
});

When I ran my application test suite, the memory usage went crazy. Turns out the event listeners for $terminatingCallbacks weren't being cleared out between tests, causing a memory leak.

This change clears out the $terminatingCallbacks array in the Application::flush() method.

@markokeeffe

This comment has been minimized.

Copy link
Contributor Author

markokeeffe commented Feb 10, 2020

Is there any procedure for merging fixes like this down to older versions? Our application is currently using v5.8 of the framework.

@xuanquynh

This comment has been minimized.

Copy link
Contributor

xuanquynh commented Feb 10, 2020

The failed test cases aren't related to this PR, I think.

@markokeeffe markokeeffe changed the title Ensure Application::$terminatingCallbacks are reset on Application::flush() [6.x] Ensure Application::$terminatingCallbacks are reset on Application::flush() Feb 10, 2020
@markokeeffe

This comment has been minimized.

Copy link
Contributor Author

markokeeffe commented Feb 10, 2020

@xuanquynh I was just looking at that, seems like those same tests are failing for a few builds 🤷‍♂

@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Feb 10, 2020

We're aware of the test failures. They are due to a redis serialisation configuration problem in the tests.

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Feb 10, 2020

Is there any procedure for merging fixes like this down to older versions? Our application is currently using v5.8 of the framework.

5.8 isn't supported anymore sorry.

@taylorotwell taylorotwell merged commit 18791d9 into laravel:6.x Feb 10, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.