Fix Doctrine source iterators

[ossinkine]

Subject

I am targeting this branch, because I've faced with BC break.
After refactoring source iterators here #343 there was lost detaching objects from object managers what led to memory leaks.

Changelog

### Fixed
- Memory leaks in Doctrine source iterators

[ossinkine]

@VincentLanglet Please confirm you just lost this but did not remove this intentionally

[VincentLanglet]

@VincentLanglet Please confirm you just lost this but did not remove this intentionally

Wasn't intentional indeed, since I don't even know what it does.

[VincentLanglet]

Oh.

I just see that the method detach is deprecated

@deprecated 2.7 This method is being removed from the ORM and won't have any replacement

Maybe it's the reason I removed it (?).

@greg0ire any opnion on this ?

[ossinkine]

Let's just remove all deprecated method calls but not replace them with correct functions 🤣

[VincentLanglet]

Let's just remove all deprecated method calls but not replace them with correct functions 🤣

Correct function which is ?

[ossinkine]

@VincentLanglet @franmomu Actually I've just reverted changes which breack BC but if you want I'll apply your requested changes

[VincentLanglet]

@VincentLanglet @franmomu Actually I've just reverted changes which breack BC but if you want I'll apply your requested changes

The request change for the Document manager seems easy to apply and avoid using an internal method.

I don't know how hard would be the request change for entityManager

• If you're motivated to apply the request change and rely on clear, this would be perfect
• If not, I can understand this, and I'll still approve since it fixes the BC-break.

[ossinkine]

I've replaced detach with clear

[franmomu]

To be honest, I'm not sure about this. I guess there won't be memory leaks, but maybe we are introducing some problems performance-wise. Based on how Doctrine shows how to to batch processing, the calls to EntityManager::clear() are made every X iterations, I guess clearing the whole identity map every iteration is not ideal, but as I said, maybe I'm wrong.

So IMHO, if we don't have enough knowledge about this, I would leave as it was and just replace the call to the @internal function.

[VincentLanglet]

Indeed, maybe it's safer to use

$this->query->getDocumentManager()->detach($current[0]);

for ODM

And keep

$this->query->getEntityManager()->getUnitOfWork()->detach($current[0]);

for ORM until we get some help to remove the deprecated method (maybe @greg0ire you can help us ?)

[ossinkine]

Actually the calling clear in the source iterator is safe because all other entities are not used here, it just read entities from iterator and put them in the response and that's all.

I also think that using deprecated functions is acceptable as opposed to internal ones because BC is guaranteed here. So we can just using $this->query->getDocumentManager()->detach($current[0]); and $this->query->getEntityManager()->detach($current[0]); here and will improve this in the future.

[greg0ire]

After reading that conversation, I think this PR looks fine and clear() is the way to go: doctrine/orm#7901 (comment)

Based on how Doctrine shows how to to batch processing, the calls to EntityManager::clear() are made every X iterations, I guess clearing the whole identity map every iteration is not ideal, but as I said, maybe I'm wrong.

flush() is obviously expensive because it will trigger SQL queries. clear(), on the other hand, just clears things that exist in memory. If you need more convincing, just read the code, it will take literally 30 seconds if you bear in mind that $entityName is null: https://github.com/doctrine/orm/blob/79cdcde9ec2b6ea4ca118959842df7860effa2dd/lib/Doctrine/ORM/UnitOfWork.php#L2498-L2526 The only shady thing is the event dispatching: if you have an event plugged on clear, and it's not performant, there will be issues, but I don't think that's something that happens that frequently in the wild.

[phansys]

IIUC, if we detach an object from the UoW (by calling clear()), that object will be not identified as an object handled by Doctrine anymore. So, if that object ends involved in the Doctrine domain again (during a call to persist() through an association or similar), the UoW will identify that object as a new instance.
I'm sharing this concern because I think that is not the intended behavior.

[greg0ire]

IIUC, if we detach an object from the UoW (by calling clear()), that object will be not identified as an object handled by Doctrine anymore. So, if that object ends involved in the Doctrine domain again (during a call to persist() through an association or similar), the UoW will identify that object as a new instance.

You're right, but that was the previous behavior anyway. detach and clear will result in that same behavior.

[phansys]

You're right, but that was the previous behavior anyway. detach and clear will result in that same behavior.

I didn't know we were detaching objects.

[ossinkine]

@franmomu please review

[franmomu]

Thanks @ossinkine!

[ossinkine]

@VincentLanglet @greg0ire Could anyone from you guys create a release with this fix please?

[VincentLanglet]

Sure, the PR is opened #386