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 single scalar hydrator memory leak on exception #8483

Conversation

olsavmic
Copy link
Contributor

@olsavmic olsavmic commented Feb 17, 2021

Resolves #8482, Closes #7315

I have encountered an issue with a memory leak in SingleScalarHydrator.

AbstractHydrator::hydrateAll() method does not take into account the possibility of an exception being thrown so when SingleScalarHydrator::hydrateAllData throws NoResultException/NonUniqueResultException, the event listener for onClear event is not removed and the hydrator is stuck in the memory.

It's been solved here once but not merged due to missing test cases.

Original issue: Fix memory leak in AbstractHydrator

I'll try to provide a fix and appropriate test case for this issue so it can be finally merged.

{
$this
->mockEventManager
->expects(self::at(0))
Copy link
Member

Choose a reason for hiding this comment

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

self::at is deprecated in recent versions of PHPUnit, please avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and rebased, although I do not like the replacement (I did not find any other recommended way). Say if it seems ok to you please. I might replace the the rest of self::at occurrences in this Test class if requested (although it's out of scope of this MR)

Copy link
Member

Choose a reason for hiding this comment

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

I might replace the the rest of self::at occurrences in this Test class if requested (although it's out of scope of this MR)

It would certainly be appreciated, but it's not at all required.

@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/2.8.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@olsavmic olsavmic force-pushed the fix-single-scalar-hydrator-memory-leak-on-exception branch from 7ef0463 to d684598 Compare February 18, 2021 07:44
@olsavmic olsavmic changed the base branch from 2.8.x to 2.9.x February 18, 2021 07:45
@olsavmic olsavmic force-pushed the fix-single-scalar-hydrator-memory-leak-on-exception branch 3 times, most recently from 702d96a to 04646ba Compare February 18, 2021 08:14
@olsavmic olsavmic force-pushed the fix-single-scalar-hydrator-memory-leak-on-exception branch from 04646ba to 2d5207e Compare February 18, 2021 08:33
@olsavmic olsavmic changed the base branch from 2.9.x to 2.8.x February 18, 2021 08:33
from hydrateAllData() which in turn does not call cleanup()
@olsavmic olsavmic force-pushed the fix-single-scalar-hydrator-memory-leak-on-exception branch from 2d5207e to 7971a53 Compare February 20, 2021 17:59
@beberlei beberlei added this to the 2.8.3 milestone Feb 21, 2021
@greg0ire greg0ire merged commit 672b04a into doctrine:2.8.x Feb 21, 2021
@greg0ire
Copy link
Member

Thanks @olsavmic !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix memory leak in AbstractHydrator - Follow up
4 participants