Skip to content

Loading…

DDC-136: Usage of spl_object_hash in UnitOfWork is dangerous #1978

Closed
doctrinebot opened this Issue · 9 comments

1 participant

@doctrinebot

Jira issue originally created by user nicokaiser:

UnitOfWork uses splobject_hash to identify objects. This breaks horribly as spl_objecthash is not unique when using it in different scopes.

See this example:
http://pastebin.com/d5e4825b0

When creating and destroying (persisting and removing) objects in sub functions, UnitOfWork gets confused.
I don't think this is a special case, so I consider this as a Blocker...

@doctrinebot

Comment created by romanb:

Of course we know how splobjecthash works and we dont see any problems with it with regards to Doctrine. There is never the same object hash on two different objects that are alive at the same time. Doctrine keeps references to all objects its manages. If they get detached then Doctrine does not care about them.

Please show an example where the splobjecthash behavior, or more concretely, the fact that hashes from destroyed objects are reused, is a problem for Doctrine.

@doctrinebot

Comment created by romanb:

To clarify, there might be (a) bug(s) where the UnitOfWork wrongly keeps entries in its internal arrays with the splobject_hash of an object it does no longer care about. But this is then rather a bug in our code and not a problem that is inherent to the behavior of spl_objecthash.

As long as Doctrine (and the UnitOfWork) cares about an object, it keeps a reference to it, hence the hash is not reused by any new objects. When Doctrine no longer cares bout an object, all entries of that object should be wiped from the UnitOfWork internal arrays and then it does not matter that later a new object gets the same hash again.

I hope I explained that well.

Thanks for your help.

@doctrinebot

Comment created by nicokaiser:

Ok, you are right, but then UnitOfWork keeps internal entries when it shouldn't.

Example:
http://pastebin.com/d71a638f8

(The both User objects in the highlighted linesget the same object hashes, the last pesist() thinks the object is detached and fails)

@doctrinebot

Comment created by @beberlei:

Btw, why dont we use SplObjectStorage at this point? Since 5.3 it has some (undocumented) behaviour that goes like:

$entity = new Entity();
$storage = new SplObjectStorage()
$storage[$entity] = array('foo' => 'bar');

$foo = $storage[$entity]['foo'];

This might save some additional performance, because I think SplObjectStorage uses the PHP internal object pointer and does not need to calculate the splobjecthash().

@doctrinebot

Comment created by romanb:

@Nico: Thanks for the example, I will look into it.

@Benjamin: I've played with the idea of SplObjectStorage several times already. It has some disadvantages. First, contrary to your and my first belief it is slower, not faster than using an array with splobjecthash. Secondly it results in additional references to the objects that you need to be aware of and can potentially prevent the objects from being garbage collected.

I mean if I store some associated information to an entity with the splobjecthash => value information there is no reference to the actual object. Using SplObjectStorage would result in object references being stored. I've always tried to keep the locations where Doctrine keeps references to objects to a minimum to not provoke unnecessary potential memory leaks.

If you would like to make your own performance tests of SplObjectStorage vs array + splobjecthash I would be glad to see your results!

@doctrinebot

Comment created by romanb:

@Nico: The issue you mentioned should be fixed now. I reproduced it in the sandbox as well as in the tests (with some effort). So we have test coverage for this case as well at least. If you find any more of such things feel free to open new issues.

Thanks for your help on improving Doctrine.

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot

Comment created by @beberlei:

@Roman: the reference issue is a serious drawback of SplObjectStorage, good that the bug could be fixed with splobjecthash nonetheless. :-)

@doctrinebot

Comment created by gamesh:

@doctrinebot doctrinebot added this to the 2.0-ALPHA3 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.