Avoid creating unmanaged proxy instances for referred-to entities during merge()
#10791
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpdude
changed the title
merge()
mpdude
force-pushed
the
understand-3037
branch
3 times, most recently
from
f96dcfa
to
4a16b33
Compare
|
@Ocramius Your thoughts would be very welcome |
|
Blocked by #10790 |
This was referenced Jun 23, 2023
greg0ire
reviewed
Jun 23, 2023
…ing merge() This PR tries to improve the situation/problem explained in doctrine#3037: Under certain conditions – there may be multiple and not all are known/well-understood – we may get inconsistencies between the `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` and `\Doctrine\ORM\UnitOfWork::$identityMap` arrays. Since the `::$identityMap` is a plain array holding object references, objects contained in it cannot be garbage-collected. `::$entityIdentifiers`, however, is indexed by `spl_object_id` values. When those objects are destructed and/or garbage-collected, the OID may be reused and reassigned to other objects later on. When the OID re-assignment happens to be for another entity, the UoW may assume incorrect entity states and, for example, miss INSERT or UPDATE operations. One cause for such inconsistencies is _replacing_ identity map entries with other object instances: This makes it possible that the old object becomes GC'd, while its OID is not cleaned up. Since that is not a use case we need to support (IMHO), doctrine#10785 is about adding a safeguard against it. In this test shown here, the `merge()` operation is currently too eager in creating a proxy object for another referred-to entity. This proxy represents an entity already present in the identity map at that time, potentially leading to this problem later on.
greg0ire
reviewed
Jun 23, 2023
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
greg0ire
approved these changes
Jun 23, 2023
greg0ire
reviewed
Jun 23, 2023
SenseException
approved these changes
Jun 25, 2023
|
Thanks @mpdude ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to improve the situation/problem explained in #3037:
Under certain conditions – there may be multiple and not all are known/well-understood – we may get inconsistencies between the
\Doctrine\ORM\UnitOfWork::$entityIdentifiersand\Doctrine\ORM\UnitOfWork::$identityMaparrays.Since the
::$identityMapis a plain array holding object references, objects contained in it cannot be garbage-collected.::$entityIdentifiers, however, is indexed byspl_object_idvalues. When those objects are destructed and/or garbage-collected, the OID may be reused and reassigned to other objects later on.When the OID re-assignment happens to be for another entity, the UoW may assume incorrect entity states and, for example, miss INSERT or UPDATE operations.
One cause for such inconsistencies is replacing identity map entries with other object instances: This makes it possible that the old object becomes GC'd, while its OID is not cleaned up. Since that is not a use case we need to support (IMHO), #10785 is about adding a safeguard against it.
In this test shown here, the
merge()operation is currently too eager in creating a proxy object for another referred-to entity. This proxy represents an entity already present in the identity map at that time, potentially leading to this problem later on.Cross-reference #3037 (comment).
Closes #7407, closes #8994.
Not enough information in #3037 to tell whether it would be fixed.