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

[2.x] Uses NullableShutdownStrategy on tests #493

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nunomaduro
Copy link

@nunomaduro nunomaduro commented Sep 27, 2022

This pull request fixes a memory leak of this package with Laravel's unit tests, where the Illuminate\Foundation\Application instance is being leaked to PHP's internal shutdown function.

In other words, Laravel often creates application instances during tests, and bugsnag is registering (for each new test case) a new shutdown function with those application instances.

With this fix, we're able to decrease the memory usage of a 600 test suite by 14% percent:

Tests: 600
Before: 190.50 MB
After: 168.50 MB

Note, #65 equally needs to merged and tagged.

@luke-belton
Copy link
Member

Hi @nunomaduro - could you explain a bit more about your use case here please? 600 tests sound like a lot to have a direct dependency on Bugsnag. Is there no way for you to isolate those tests from Bugsnag and/or mock Bugsnag to avoid that dependency?

@luke-belton luke-belton added the awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. label Oct 4, 2022
@johnkiely1
Copy link
Member

Hi @nunomaduro, have you seen the message from luke-belton above? We would really need to understand your use case in order to consider this PR.

@Gavrisimo
Copy link

@luke-belton @johnkiely1 I think this originated in this issue: laravel/framework#44214

@johnkiely1 johnkiely1 added backlog We hope to fix this feature/bug in the future and removed awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. labels Jan 16, 2023
@johnkiely1
Copy link
Member

We will look to review this PR when priorities allow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog We hope to fix this feature/bug in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants