Bugzilla – Full Text Bug Listing
| Summary: | Containment Cycle Detection should also work for non branching configurations | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Martin Fluegge <martin.fluegge> | ||||||||||||
| Component: | cdo.core | Assignee: | Martin Fluegge <martin.fluegge> | ||||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | Flags: | stepper:
review+
|
||||||||||||
| Version: | 4.0 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Martin Fluegge
Created attachment 173322 [details]
Patch v1
I found a solution which works also with non-branching config. But this is rather a poor hack. Or in other words, this is anything but a clean solution. So please, do not commit it ;)
It also performs poor on legacy but it shows the idea behind it.
The problem is, as I and you guessed, that the equals method fails because the CDOID does not match the CDOIDRevisionDeltaLockWrapper. As a result the lockedObjects HashMap in the TransactionCommitContext will contain a Wrapper and a CDOID for the same ID (OID7 in our test). When the key is retrieved from the lockManager the CDOID will be found first and thus it is no CDOIDRevisionDeltaLockWrapper the delta will not be compared and no check will be performed for locked parents.
The patch is not the final solution. I think we should discuss this if there is some time.
Created attachment 173685 [details]
Pacth v2_1
After looking at this topic again my first patch does not seem to be that bad. I played a bit with a special LockContainer object (Patch v2_1). But I am not sure whether this is worth the effort. Patch v2_2 is simpler and does not need to break API. We should discuss both patches.
Created attachment 173686 [details]
Patch v2_2
Created attachment 175433 [details] Patch V3 O.k. I had a look at the Set.add() method again because I couldn?t believe that the java doc is so complete wrong. And than I found it...The java doc is correct and add() does *not* overwrite the value. The trick will be visible if you look at the implementation a map.put(). There the *value* will be replaced, but nut the *key*. Because Set.add() simple delegates to map.put() to store the value for the key of the map, the key remains unchanged - the value is not overwritten. With the test we wrote we managed to fool ourselves because we overwrote equals() to return true in any cause. When we checked that our second object is in the set (our proof that the value was overwritten) we also should have checked whether the list contains the other object, too...what it did due to our equals() method ;) O.k. back to the topic. That?s why I locked the references not before but after the containment check. I also changed the test for the initial bugzilla (Bug 316444) slightly because it was not 100% reliable. (In reply to comment #4) > With the test we wrote we managed to fool ourselves because we overwrote > equals() to return true in any cause. True. I wanted to (but did not) use assertSame() rather than assertEquals() ;-( Created attachment 176083 [details]
Patch v4 - ready to be committed
Just a minor reformat...
Patch v4 - Committed to HEAD. Available in R20110608-1407 |