[8.x] fix HandleExceptions memory leak in tests #40592
Conversation
donnysim
changed the title
|
When you say that currently it is not cleaned up until all tests are done - do you mean it is cleaned up after each test class is finished or not until the entire suite is done running? |
|
Test suite. The previous handlers will be kept as long as the PHP process is alive and increase with each test, or we could say, every application instantiation that executes the HandleException@bootstrap. This change makes sure that PHP doesn't hold it in the error, exception or in shutdown handlers stack, preventing them from being GC. |
|
Failing PHP 8.1 tests are not related to my change but can fix them too if need be. |
|
@donnysim I think these failures are on your branch since they're not happening on 8.x so can you rebase maybe? |
|
@driesvints don't really get it, the 8.x branch does have this issue in tests even locally running with 8.1 https://github.com/laravel/framework/blob/8.x/tests/Support/SupportJsTest.php#L56 It doesn't matter if I run it with or without the change, I still get this error. Kind of confused why it doesn't occur in other pull requests. |
|
I think something was wrong with the error/deprecation handler prior to this PR and that this PR exposed that bug. The CI pipeline should be failing on the 8.x branch too (which currently does not happen) as there's no return type hint nor the return type will change annotation on that Maybe it has something to do with the order in which the tests are executed on the CI. |
|
@X-Coder264 that would make sense, all other |
|
@X-Coder264 I don't understand. Surely those tests would fail on 8.x right now as well? This shouldn't happen.. |
|
@driesvints Yeah, they should fail on 8.x but they don't which is weird. After fixing the return type new deprecation exceptions popped up on the latest pipeline: |
If I clone the 8.x branch and run those tests with PHP 8.1, it fails at the same place and more. if you just run the file <?php
declare(strict_types=1);
$data = new class() implements JsonSerializable
{
public $foo = 'not hello';
public $bar = 'not world';
public function toJson($options = 0)
{
return json_encode(['foo' => 'hello', 'bar' => 'world'], $options);
}
public function jsonSerialize()
{
return ['foo' => 'not hello', 'bar' => 'not world'];
}
public function toArray()
{
return ['foo' => 'not hello', 'bar' => 'not world'];
}
};as it is in the tests with PHP 8.1 it throws a deprecation warning, there's just no way it should pass in the CI. |
|
@driesvints there does seem to be a test ordering problem, didn't find which yet but if some tests run before it, then the deprecation is hidden, so something is tinkering with the exceptions and not restoring them (testing without this pull change). |
|
@driesvints it has something to do with using Orchestra\Testbench\TestCase, as soon as this is used, deprecation warnings are gone, |
|
@nunomaduro it seems like this 52e2376 commit introduced a bug where the deprecation errors are swallowed because no LogManager is registered when used with |
|
Odd because I sent in a PR recently to explicitly surface deprecation errors in CI: #40324 so I'm not sure why these were being repressed. I pinged @nunomaduro so hopefully he can shed some light here. |
|
Placing this in draft until @nunomaduro has had a chance to reply and we've resolved all issues. |
|
The main problem is that the handler just exits silently - null/void handler result is held as user handled event. If it was supposed to fallback to the default PHP exception handling in case of the deprecation error it should return false or throw an error itself: if (! class_exists(LogManager::class)) {
return false;
}
try {
$logger = $this->app->make(LogManager::class);
} catch (Exception $e) {
return false;
}otherwise no matter the |
|
@driesvints @nunomaduro I took a quick look at the state on the 8.x branch. There are two issues here that need to be addressed, one concerning integration tests where the framework is booted and another which affects unit tests (which do not boot the framework). After #39219 all deprecations for integration tests were silenced (both for user apps as well as for Laravel's own test suite).
So that solves the issue of why some deprecations were left unnoticed in integration tests (you can confirm this is the cause of the problem by calling Now onto why some deprecations were not noticed in unit tests (like the one in PHPUnit is properly configured to convert deprecations to exceptions which happens in the PHPUnit ErrorHandler which is registered on the test run, but PHPUnit does not register its error handler if a previous error handler was already configured -> https://github.com/sebastianbergmann/phpunit/blob/9.5.13/src/Util/ErrorHandler.php#L131-L146 This means that if an integration test runs before an unit test it sets the Laravel error handler and then when the unit test runs PHPUnit does not register its own error handler and because Laravel's error handler does not throw an exception anymore the test turns out green instead of red. The fix here would be to always clear the error handler that gets set during the framework boot (by calling |
|
Alternate fix which should resolve all problems -> #40639 @driesvints @nunomaduro Any feedback on that PR is greatly appreciated. @donnysim Can you also please confirm that it solves your memory leak problem? Thank you. |
|
@X-Coder264 @driesvints with one small change the new pull is looking good. Closing this in favor of #40639. |
Fix for #40554 . By introducing a separate global handler we can avoid keeping every HandleExceptions with its own Application instance in memory and it can be cleaned up after each test instead of after all tests are done.
Couldn't think of any other way to deal with it without introducing a separate class.