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

Fix memory leak in ExceptionWrapper #5012

Merged
merged 1 commit into from Aug 23, 2022
Merged

Fix memory leak in ExceptionWrapper #5012

merged 1 commit into from Aug 23, 2022

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jul 8, 2022

Currently ExceptionWrapper stores the original exception forever and prevents it to be GCed.

This is causing not only memory to grow/leak, but also if the original exception has a reference to an object, the object is not released as well. Such scenario is not uncommon, as exception stack trace/frame can contain object arguments.

In consequence when the unreleased object is a some limited resource like DB connection, with a lot of wrapped/unreleased exceptions, the resource can be exhausted by a few failing tests making the other tests fail because of them.

This fix maintains the shallow backtrace as WeakReference property is not expanded during backtrace dump.

No BC break introduced.

@@ -109,14 +116,23 @@ public function getOriginalException(): ?Throwable
*/
private function originalException(Throwable $exceptionToStore = null): ?Throwable
{
static $originalExceptions;
// drop once PHP 7.3 support is removed
if (PHP_VERSION_ID < 70400) {
Copy link
Contributor Author

@mvorisek mvorisek Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop this code when merging to phpunit 10.x, but fix should target 9.5.x at least/latest stable

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #5012 (bd1a437) into 9.5 (8885568) will decrease coverage by 0.02%.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##                9.5    #5012      +/-   ##
============================================
- Coverage     83.82%   83.79%   -0.03%     
- Complexity     4615     4618       +3     
============================================
  Files           272      272              
  Lines         11470    11474       +4     
============================================
  Hits           9615     9615              
- Misses         1855     1859       +4     
Impacted Files Coverage Δ
src/Framework/ExceptionWrapper.php 89.18% <42.85%> (-10.82%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mvorisek
Copy link
Contributor Author

I have updated the PR description to understand the real issue better. Please merge this fix into at least phpunit 9.5.

For next major release, is there any reason to not drop ExceptionWrapper completely?

@sebastianbergmann
Copy link
Owner

For next major release, is there any reason to not drop ExceptionWrapper completely?

The ExceptionWrapper has been removed in the main branch.

@mvorisek
Copy link
Contributor Author

The ExceptionWrapper has been removed in the main branch.

Thank you ❤️

To fix the memory leaks in phpunit 9.5, can this PR be merged into 9.5.x branch? No BC break should be involved.

@sebastianbergmann sebastianbergmann merged commit 44e8bb0 into sebastianbergmann:9.5 Aug 23, 2022
@mvorisek mvorisek deleted the fix_mem_leak branch August 23, 2022 16:14
@sebastianbergmann sebastianbergmann added the type/performance Issues related to resource consumption (time and memory) label Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/performance Issues related to resource consumption (time and memory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants