[DDC-2785] Add failing test

[flack]

Add a failing test for

http://www.doctrine-project.org/jira/browse/DDC-2785

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2786

We use Jira to track the state of pull requests and the versions they got
included in.

[beberlei]

The test is wrong becaues of this line, when calling ->persist() the entity is managed, not NEW. You can move this line into the if condition below and then it passes green.

Hash collisions are ok, Doctrine fixes them by cleaning the internals of the UoW up.

[beberlei]

See my code comment for why this is wrong. Can you explain a little bit what prompted you to create this PR and the issue?

[flack]

@beberlei You're right about the test being wrong, I looked at the code in UoW again, and this can't return STATE_NEW.

The problem I was trying to reproduce is that I've encountered situations where I instantiated a new object and EM would tell my that it is already managed when calling persist (which later on usually causes exceptions when calling flush), at least that is what I found when stepping through my code (and Doctrine's) with a debugger.

In one case, a new object instance which is created in a prePersistcallback had the same oid as another already deleted item (from a different testcase IIRC). You can see the workaround here:

https://github.com/flack/midgard-portable/blob/master/src/midgard/portable/storage/subscriber.php#L42

But in the other case, it's just a new object instance passed to Doctrine via persist, you can see the workaround here:

https://github.com/flack/midgard-portable/blob/master/src/midgard/portable/storage/objectmanager.php#L38

I've played around with the unit test a while, but I can't get it to behave the same way as my code, and even there, it only when happens when I run a pretty large test suite (of a depending project) against it. So there is some difference, but I don't know what it could be. But the workarounds linked above fix the problem, and at least in the first case, I can't see how it could be an error in my code (but I've been known to be wrong before :-)

[flack]

@beberlei: I think I finally figured out how to reproduce the problem: The trick was to get a reference on the object. This somehow provokes the situation described above. But of course, I might still be missing something, so if you could look over the test and tell me if the sequence of calls I'm using is supposed to work the way I expect them to, it would be most helpful!

[flack]

ping

I don't mean to stress, but it would really be helpful if someone could comment on this. I've been chasing sporadic failures in my Doctrine code for a long while now, and AFAICT they usually derive from some variation of the code above.

[Ocramius]

Not needed anymore

[flack]

I've fixed the CS problems mentioned by @Ocramius now. Let me know if you need more information or if I can help with debugging somehow

[flack]

P.S.: I don't think that the $object itself is present in UoW at that point any more. Because if it were, then spl_object_hash would not return a colliding value (unless there's some bug in PHP). To me, it looks more as if the object hashes are not cleaned up correctly from the various arrays in UoW. And I'm fairly sure it has something to do with reference proxies, because if you remove the line

$x = $this->_em->getReference(get_class($phone), $phone->phonenumber);

then the test passes

[flack]

@stof I removed uniqid now and replaced it with a simple counter (which may also make it a bit easier to see which objects collide). The test result is still the same, though

[flack]

Just to let you know: I was trying to run the test again against current master, hoping that maybe this was fixed by #5689, but the test still fails, i.e. there still seems to be a way for hash collisions to occur

[mpdude]

Your test is using CmsPhonenumber, which uses the given number as the entity ID (application-provided IDs).

In your test, you first execute

$x = $this->_em->getReference(get_class($phone), $phone->phonenumber);

The $x object returned is now the one and only object legetimately representing the given phone number. But, you then persist() another object using the same ID.

This leads to an internal inconsistency in the UoW, since the $phone object is taking the place in the identity map, but we have two entries in \Doctrine\ORM\UnitOfWork::$entityIdentifiers (for different OIDs) resolving to the same ID hash.

#10785 would have spotted this; it rejects your $phone object when passing it to persist().

So my take would be that the test shows why it would be good to have the #10785 check, but it does not demonstrate a problem with object id collistions in a valid way.

[flack]

@mpdude AFAIR the situation I had in my real code was that I loaded some stuff with querybuilder, and associations for the loaded entites got automatically initialised as references. Then, when later loading those entities the object hash collisions cropped up (there may have been some detach() calls happening in between). I suppose I could put some effort into making the testcase behave a bit more like what I observed in my actual code, but then again, this PR has been open for almost 10 years now, so I kind of doubt it's worth the effort

[Ocramius]

The reason why this has staid open for so long is precisely because the reproducer was not deterministic.

As much as I appreciate the effort put into trying to hunt the bug, we need precise pinpointing to avoid future regressions and other related bugs: once things get thrown over the fence, we can't really have something that fails randomly, or for certain contributors only.

@mpdude if you think your approach will uncover the bug with precision, do feel free to send a patch.

@flack I'm still reading here, btw, it's just not my job to do so :D

[Ocramius]

As for current state: this still targets travis-ci, so I guess it's very (very!) outdated anyway.

Your call, @flack: even if you decide to close here, it is still a breadcrumb for anyone picking this up in future.

Meanwhile, Jira has been shut down many years ago, so initial context is lost.

[flack]

@Ocramius DDC-2785 also got migrated to github back then: #3533, so original context can still be viewed if it's interesting to anyone.

I'll keep the PR open for now, maybe I'll take another stab at a different approach (at some point in the future), but I'm not sure this can even be fully deterministically reproduced. I mean, the hashes are more or less randomly generated by PHP internally, so whether or not you get a collision will always involve some chance, no?

All I know is that the collisions still happen in real life, I have some logging for it in my code and just checked a smallish server (only a handful of daily active users) and I get around 5 collisions every day.

[Ocramius]

I mean, the hashes are more or less randomly generated by PHP internally, so whether or not you get a collision will always involve some chance, no?

Identifiers are deterministic, AFAIK: what we're missing is something like a GC causing an identifier reuse, perhaps.

Anyway, I also don't have better answers, at this stage.

[mpdude]

I think #3037 is the main issue dealing with these spurious hash collisions, and yes, it's still lacking a reproducer.

What I wanted to say is that I believe this code here is not a valid reproducer, since it uses the ORM in a wrong manner (see comment). #10785 is a WIP adding a check that would call this out.

[greg0ire]

The right way to fix this would be to use weak maps, wouldn't it?

[mpdude]

Regarding the code in this PR, I think it’s a user error to first ask the EM for a reference for given ID and then pass another object to persist() using the same ID.

Weak maps might help simplify state management in the UoW, but this code here needs to be fixed on the user side.

[flack]

closing in favor of #10797 / #10785