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

PHPLIB-501: Prevent memory leaks from generators #694

Merged
merged 2 commits into from
Nov 15, 2019

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 8, 2019

https://jira.mongodb.org/browse/PHPLIB-501

This was discovered while investigating doctrine/mongodb-odm#2092. The different behaviour can be seen in action here: https://3v4l.org/UIfIb. Note that when we don't specifically unset the generator, it stays around much longer even though the wrapping iterator has already been garbage collected.

@alcaeus alcaeus requested a review from jmikola November 8, 2019 08:13
@alcaeus alcaeus self-assigned this Nov 8, 2019
@alcaeus alcaeus force-pushed the fix-generator-memory-leak branch 2 times, most recently from e1fc8be to 6c40fd7 Compare November 11, 2019 08:34
@@ -61,8 +64,7 @@ class CachingIterator implements Countable, Iterator
*/
public function __construct(Traversable $traversable)
{
$this->iterator = $this->wrapTraversable($traversable);
$this->storeCurrentItem();
$this->iterator = new IteratorIterator($traversable);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to just prepare the iterator here and store the current item? That might allow you to remove the prepareIterator() calls from other methods.

As-is, there might be a subtle behavioral change, since the original implementation did rewind the traversable, create the generator, and iterate once to store the current item?

@jmikola jmikola changed the title PHPLIB-501: Remove reference to generators on iterator destruction PHPLIB-501: Prevent memory leaks from generators Nov 12, 2019
@jmikola
Copy link
Member

jmikola commented Nov 12, 2019

Renamed the issue to reflect the actual change and title in JIRA.

Also, re: your comment in doctrine/mongodb-odm#2100 (comment), can you elaborate on the subtle behavioral change by switching to IteratorIterator? Does it relate to my earlier comment about removing the prepareIterator() call?

@alcaeus
Copy link
Member Author

alcaeus commented Nov 12, 2019

That is correct. When using the generator, we immediately start iterating the wrapped traversable. When switching to IteratorIterator, we can still do that (by calling rewind in the constructor), but we may want to defer starting iteration of the wrapped traversable until the wrapping iterator is starting its iteration.

@alcaeus alcaeus force-pushed the fix-generator-memory-leak branch 3 times, most recently from a75cc5a to 0015e6d Compare November 13, 2019 14:18
@alcaeus alcaeus force-pushed the fix-generator-memory-leak branch from 0015e6d to 964954e Compare November 14, 2019 07:43
@alcaeus
Copy link
Member Author

alcaeus commented Nov 14, 2019

PR adapted to replicate previous behaviour: iteration of the traversable starts when CachingIterator is constructed, not when iteration of CachingIterator starts.

@alcaeus alcaeus requested a review from jmikola November 14, 2019 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants