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 AbstractHydrator #7315

Closed
wants to merge 1 commit into from
Closed

Fix memory leak in AbstractHydrator #7315

wants to merge 1 commit into from

Conversation

zlikavac32
Copy link

Method hydrateAll() does not take into account possible
exception from hydrateAllData() which in turn does not call
cleanup()

This is the issue when single scalar is used since it throws exceptions. To demo this, one can just select from an empty table and see how the memory rises.

In this example, few test cases were provided. One that hydrates single scalar result and an other that hydrates an array. In the second case, the memory is constant.

$ for i in 1000 10000 100000; do ./run $i; done 
int(1000)
string(4) "3 MB"
string(5) "6 KB"
int(10000)
string(5) "13 MB"
string(5) "6 KB"
int(100000)
string(6) "112 MB"
string(5) "6 KB"

After the fix, situation is as following

$ for i in 1000 10000 100000; do ./run $i; done
int(1000)
string(4) "2 MB"
string(5) "6 KB"
int(10000)
string(4) "2 MB"
string(5) "6 KB"
int(100000)
string(4) "2 MB"
string(5) "6 KB"

What do you suggest, how to add tests? Repeat the same query a few hundred times and set a reasonable threshold like 10MB?

Code used to test this can be found at https://github.com/zlikavac32/doctrine-memory-leak-demo

Method hydrateAll() does not take into account possible
exception from hydrateAllData() which in turn does not call
cleanup()
@Ocramius
Copy link
Member

Fix looks valid.

A correct test would:

  1. perform the crash twice
  2. measure memory
  3. perform the crash another time
  4. verify that memory usage didn't increase over a certain amount

To make this easily measurable and resistant to flakiness, a fake resultset that produces large text to be hydrated into an entity property could be used.

This should probably be a unit test around hydration: see https://github.com/doctrine/doctrine2/blob/847d5bc6660c5e35fee872f5326cc29261eca197/tests/Doctrine/Tests/ORM/Hydration/AbstractHydratorTest.php

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

Successfully merging this pull request may close these issues.

None yet

4 participants