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

DDC-3146 Remove onClear event listener from abstract hydrator in cleanup function, otherwise causing hydrator GC to not kick in #1515

Closed

Conversation

emiel-silverstreet
Copy link
Contributor

When the iterate function is invoked an event listener is added, but never removed. This will keep a reference to the abstract hydrator in the event manager, which will prevent the php garbage collector from removing the instance. When used multiple times this will create a memory leak because the event manager will be filled with references to unused hydrators. This fix will remove the event listener in the cleanup function which is invoked when the hydrator is finished, allowing the class to be garbage collected.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3914

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Copy link
Member

@emiel-silverstreet the fix looks good, but:

  • the test is mocking too many dependencies, and it is actually easier to follow the test if we have an actual integration test
  • we could just count the number of event listeners attached to the event manager (in the test), instead of keeping the counters

@Ocramius Ocramius self-assigned this Jun 24, 2017
@Ocramius Ocramius added the Bug label Jun 24, 2017
@Ocramius Ocramius added this to the 2.6.0 milestone Jun 24, 2017
Ocramius added a commit that referenced this pull request Jun 24, 2017
…' into 2.5

Close #1515

This is a backport of the original PR - the same patch should land in `master` too, after a cleanup
Ocramius added a commit that referenced this pull request Jun 24, 2017
@Ocramius Ocramius closed this in 807f142 Jun 24, 2017
@Ocramius
Copy link
Member

I manually merged this patch after a rebase:

@Ocramius Ocramius modified the milestones: 2.5.7, 2.6.0 Jun 24, 2017
@Ocramius Ocramius changed the title [DDC-3146] Remove event listener from abstract hydrator in cleanup function DDC-3146 Remove onClear event listener from abstract hydrator in cleanup function, otherwise causing hydrator GC to not kick in Jul 27, 2017
@echo511
Copy link
Contributor

echo511 commented Aug 13, 2017

Overlooked method hydrateAll()? PR: #6623

Ocramius added a commit that referenced this pull request Aug 16, 2017
…ns-up-on-unit-of-work-clear-2.5' into 2.5

Close #6623
Ocramius added a commit that referenced this pull request Aug 16, 2017
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.

4 participants