[9.x] Fixes Foundation\Application instance leaking between feature tests

[nunomaduro]

After hours of investigation, here is a pull request that fixes #40592 yet takes a different approach from #40639.

So, to be clear, as described here #40639, this pull request attempts to fix a memory leak between Laravel feature tests.

// 10,000 feature tests:
Time: 00:32.837, Memory: 114.00 MB // Before
Time: 00:32.797, Memory: 100.00 MB // After

This issue relies on the Foundation\Application instance that never gets clean from memory by PHP's garbage collector, as the HandleExceptions::$app property gets used by set_error_handler, set_exception_handler, and register_shutdown_function as described on HandleExceptions::bootstrap:

// $this contains a $this->app property that points to the `Foundation\Application` instance
set_exception_handler([$this, 'handleException']); 

Now, the solution proposed in this pull request is simply about making the HandleExceptions::$app a static property in HandleExceptions. And this way, the previous HandleExceptions::$app gets overridden every new Laravel test and, of course, collected by PHP's garbage collector.

In addition, this pull request offers an HandleExceptions::forgetApp that gets used by our own TestCase feature test class, to ensure the error handler set by the framework never gets used even if PHPUnit doesn't set its own error handler - it may happen on certain PHPUnit options.

With this solution, on the contrary of #40639, we don't play with set_error_handler, set_exception_handler, register_shutdown_function handlers. These internal handlers are used by multiple vendors, and PHPUnit itself depends on the PHPUnit options. Making this pull request, a simpler and less error-prone solution compared with #40639.

Going to debate this pull request with the original authors, and Taylor. If we agree on this solution, here are the remaining tasks:

• Write tests
• Pull request HandleExceptions::forgetApp to TestBench. ([7.x] Uses HandleExceptions::forgetApp() to avoid memory leaks orchestral/testbench-core#75)

Closes #40639.

[donnysim]

With this pull request:

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

"62 mb"
"70 mb"
"76 mb"
"84 mb"
"94 mb"
"102 mb"
"110 mb"
"118 mb"
"124 mb"
"132 mb"


Time: 01:35.742, Memory: 132.00 MB

OK (10940 tests, 10940 assertions)

Process finished with exit code 0

with #40639 pull request:

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

"60 mb"
"64 mb"
"70 mb"
"74 mb"
"82 mb"
"88 mb"
"92 mb"
"98 mb"
"102 mb"
"106 mb"


Time: 01:35.817, Memory: 106.00 MB

OK (10940 tests, 10940 assertions)

Process finished with exit code 0

The problem without restoring the exception handlers is that we still keep 10k HandleExceptions instances in memory (but got rid of the Application part), which does still make quite the difference the more tests there are.

[X-Coder264]

I don't have time to look into this more deeply today, but like @donnysim showed, this still does not fix the memory leak problem.

I'd have to test this but since the error handlers are not restored (as the memory leak is still there) I'm not sure this fixes all the other problems that the previous alternative fixed.

Also IMO a major drawback for this alternative is that it has a breaking change and as such targets 9.x and I think that any proposed solution should target 8.x as the bug is present there as well and there's no reason to fix this only on 9.x unless it's absolutely impossible to fix it on 8.x without introducing breaking changes (which I guess it isn't since the previous alternative does not have a breaking change, or at the very least it shouldn't break anything).

[nunomaduro]

@donnysim I understand that my pull request only addresses one memory leak "the app instance", and not also the multiple exception handlers. And, it's intentionally targeting 9.x, as you mentioned, is a breaking change.

It addresses the "the app instance" memory leak, as it is an easy fix for little benefit. Yet, it does not address the "handler" memory leak as it is too big of a change - for a little benefit. Just looking at #40639, we would need to test the ramifications of those "App::terminate" changes on Vapor, Octane, and think about how possible those changes would affect thousands of existing test suites/apps.

Lets wait for feedback from Taylor, before moving forward with this or #40639.

[donnysim]

@nunomaduro I understand, I had a hunch it would impact Octane and Vapor and was just hoping it doesn't. What about something like my original #40592 approach then? By registering a separate global as the error handler we just swap the HandleExceptions instance and only one lingers at any given time without making any bigger impact on how it works originally., though I'm not sure if that also impacts Octane and Vapor.

[nunomaduro]

@donnysim The pull request #40592 introduces a new bug:

• Feature test 1: PHPUnit loads its exception handler, Laravel loads its exception handler. ✅
• Feature test 2: PHPUnit loads its exception handler, Laravel won't re-load its exception handler. ❌

In this last, because PHPUnit exception handler will act first, you will start to see PHP deprecations on the terminal.

[donnysim]

@nunomaduro I was not proposing picking mine, just maybe it's possible to combine yours with the "single global" handler to get the best of both worlds. I'm probably misunderstanding something so I'll leave it at that, lets wait and see what Taylor thinks.

[taylorotwell]

Mark as ready when you are totally ready for it to be merged, etc.

[nunomaduro]

@taylorotwell Ready.