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

[GH-8410] Fix memory leak in new toIterable and state bug. #8467

Merged
merged 6 commits into from
Feb 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lib/Doctrine/ORM/AbstractQuery.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Doctrine\ORM\Internal\Hydration\IterableResult;
use Doctrine\ORM\Mapping\MappingException as ORMMappingException;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\Query\QueryException;
use Doctrine\ORM\Query\ResultSetMapping;
use Doctrine\Persistence\Mapping\MappingException;
use Traversable;
Expand Down Expand Up @@ -998,7 +999,12 @@ public function toIterable(iterable $parameters = [], $hydrationMode = null): it
$this->setParameters($parameters);
}

$rsm = $this->getResultSetMapping();
$rsm = $this->getResultSetMapping();

if ($rsm->isMixed && count($rsm->scalarMappings) > 0) {
throw QueryException::iterateWithMixedResultNotAllowed();
}

$stmt = $this->_doExecute();

return $this->_em->newHydrator($this->_hydrationMode)->toIterable($stmt, $rsm, $this->_hints);
Expand Down
17 changes: 14 additions & 3 deletions lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

use function array_map;
use function array_merge;
use function count;
use function end;
use function in_array;
use function trigger_error;
Expand Down Expand Up @@ -165,8 +166,6 @@ public function toIterable(Statement $stmt, ResultSetMapping $resultSetMapping,

$this->prepare();

$result = [];

while (true) {
$row = $this->_stmt->fetch(FetchMode::ASSOCIATIVE);

Expand All @@ -176,9 +175,17 @@ public function toIterable(Statement $stmt, ResultSetMapping $resultSetMapping,
break;
}

$result = [];

$this->hydrateRowData($row, $result);

yield end($result);
$this->cleanupAfterRowIteration();

if (count($result) === 1) {
yield end($result);
} else {
yield $result;
}
}
}

Expand Down Expand Up @@ -274,6 +281,10 @@ protected function cleanup()
->removeEventListener([Events::onClear], $this);
}

protected function cleanupAfterRowIteration(): void
{
}

/**
* Hydrates a single row from the current statement instance.
*
Expand Down
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ protected function cleanup()
$this->_uow->hydrationComplete();
}

protected function cleanupAfterRowIteration(): void
{
$this->identifierMap =
$this->initializedCollections =
$this->existingCollections =
$this->resultPointers = [];
}

/**
* {@inheritdoc}
*/
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/ORM/Query.php
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,8 @@ public function getMaxResults()
* Executes the query and returns an IterableResult that can be used to incrementally
* iterated over the result.
*
* @deprecated
*
* @param ArrayCollection|mixed[]|null $parameters The query parameters.
* @param string|int $hydrationMode The hydration mode to use.
*
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Query/QueryException.php
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,11 @@ public static function iterateWithFetchJoinNotAllowed($assoc)
);
}

public static function iterateWithMixedResultNotAllowed(): QueryException
{
return new self('Iterating a query with mixed results (using scalars) is not supported.');
}

/**
* @return QueryException
*/
Expand Down
2 changes: 0 additions & 2 deletions tests/Doctrine/Tests/ORM/Functional/AdvancedDqlQueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,6 @@ public function testSelectSubselect(): void
$this->assertEquals('Caramba', $result[0]['brandName']);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
}

public function testInSubselect(): void
Expand Down
4 changes: 4 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/QueryIterableTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public function testAlias(): void
$users = $query->getResult();
self::assertCount(1, $users);

$this->assertEquals('gblanco', $users[0]['user']->username);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
Expand Down Expand Up @@ -60,6 +62,8 @@ public function testAliasInnerJoin(): void
$users = $query->getResult();
self::assertCount(1, $users);

$this->assertEquals('gblanco', $users[0]['user']->username);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
Expand Down
45 changes: 45 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,51 @@ public function testIterateResultIterativelyBuildUpUnitOfWork(): void
self::assertSame(2, $iteratedCount);
}

public function testToIterableWithMultipleSelectElements(): void
{
$author = new CmsUser();
$author->name = 'Ben';
$author->username = 'beberlei';

$article1 = new CmsArticle();
$article1->topic = 'Doctrine 2';
$article1->text = 'This is an introduction to Doctrine 2.';
$article1->setAuthor($author);

$article2 = new CmsArticle();
$article2->topic = 'Symfony 2';
$article2->text = 'This is an introduction to Symfony 2.';
$article2->setAuthor($author);

$this->_em->persist($article1);
$this->_em->persist($article2);
$this->_em->persist($author);

$this->_em->flush();
$this->_em->clear();

$query = $this->_em->createQuery('select a, u from ' . CmsArticle::class . ' a JOIN ' . CmsUser::class . ' u WITH a.user = u');

$result = iterator_to_array($query->toIterable());

$this->assertCount(2, $result);

foreach ($result as $row) {
$this->assertCount(2, $row);
$this->assertInstanceOf(CmsArticle::class, $row[0]);
$this->assertInstanceOf(CmsUser::class, $row[1]);
}
}

public function testToIterableWithMixedResultIsNotAllowed(): void
{
$this->expectException(QueryException::class);
$this->expectExceptionMessage('Iterating a query with mixed results (using scalars) is not supported.');

$query = $this->_em->createQuery('select a, a.topic from ' . CmsArticle::class . ' a');
$query->toIterable();
}

public function testIterateResultClearEveryCycle(): void
{
$article1 = new CmsArticle();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,20 @@ public function testNonUniqueObjectHydrationDuringIteration(): void
$bs = IterableTester::iterableToArray(
$q->toIterable([], AbstractQuery::HYDRATE_OBJECT)
);

$this->assertCount(2, $bs);
$this->assertInstanceOf(GH7496EntityB::class, $bs[0]);
$this->assertInstanceOf(GH7496EntityB::class, $bs[1]);
$this->assertEquals(1, $bs[0]->id);
$this->assertEquals(1, $bs[1]->id);

$bs = IterableTester::iterableToArray(
$q->toIterable([], AbstractQuery::HYDRATE_ARRAY)
);

$this->assertCount(2, $bs);
$this->assertEquals(1, $bs[0]['id']);
$this->assertEquals(1, $bs[1]['id']);
}
}

Expand Down