Community
Participate
Working Groups
When basicRemove is called on a SubsetSupersetEObjectEList the subset is updated when didRemove is called. As a result the call to subsetRemove possibly results in an inverseRemove via the subset although the API of basicRemove states that inverseRemove is not called. When using the UML model with CDO this leads to detachment of the inversely removed object during object invalidation. I wonder if it is feasible to change this behaviour by just calling basicRemove on the subset? Here is a stack that shows the issue: CDOResourceImpl.detached(EObject) line: 1664 PropertyImpl(BasicEObjectImpl).eBasicSetContainer(InternalEObject, int, NotificationChain) line: 1328 PropertyImpl(ElementImpl).eBasicSetContainer(InternalEObject, int, NotificationChain) line: 941 PropertyImpl.basicSetOwningAssociation(Association, NotificationChain) line: 1177 PropertyImpl.eInverseRemove(InternalEObject, int, NotificationChain) line: 1897 PropertyImpl(BasicEObjectImpl).eInverseRemove(InternalEObject, int, Class<?>, NotificationChain) line: 1441 SubsetSupersetEObjectContainmentWithInverseEList$Resolving<E>(EcoreEList<E>).inverseRemove(E, NotificationChain) line: 312 SubsetSupersetEObjectContainmentWithInverseEList$Resolving<E>(NotifyingListImpl<E>).remove(int) line: 740 SubsetSupersetEObjectContainmentWithInverseEList$Resolving<E>(AbstractEList<E>).remove(Object) line: 462 SubsetSupersetEObjectWithInverseResolvingEList<E>(SubsetSupersetEObjectEList<E>).subsetRemove(Object) line: 163 SubsetSupersetEObjectWithInverseResolvingEList<E>(SubsetSupersetEObjectEList<E>).didRemove(int, E) line: 255 SubsetSupersetEObjectWithInverseResolvingEList<E>(BasicEList<E>).remove(int) line: 615 SubsetSupersetEObjectWithInverseResolvingEList<E>(NotifyingListImpl<E>).doRemove(int) line: 756 SubsetSupersetEObjectWithInverseResolvingEList<E>(NotifyingListImpl<E>).basicRemove(Object, NotificationChain) line: 1040 CDOLegacyAdapter(CDOLegacyWrapper).clearEList(InternalEList<?>) line: 968 CDOLegacyAdapter(CDOLegacyWrapper).revisionToInstanceFeature(EStructuralFeature) line: 617 CDOLegacyAdapter(CDOLegacyWrapper).revisionToInstance() line: 494 CDOLegacyAdapter(CDOLegacyWrapper).cdoInternalPostLoad() line: 369 CDOStateMachine$InvalidateTransition.execute(InternalCDOObject, CDOState, CDOEvent, CDORevisionKey) line: 1322 CDOStateMachine$InvalidateTransition.execute(Object, Enum, Enum, Object) line: 1 CDOStateMachine(FiniteStateMachine<STATE,EVENT,SUBJECT>).process(SUBJECT, EVENT, DATA) line: 172 CDOStateMachine.invalidate(InternalCDOObject, CDORevisionKey) line: 508
Created attachment 267286 [details] Patch of class that accounts for basic method calls
This issue is a stopper for using UML together with CDO because it may lead to unwanted deletion ob objects during invalidation. Since I do not see a way to easily solve this issue on CDO side I have added a patch that uses basic method calls on sub\super in case it is triggered from a basic method call. I think the behaviour in case of removal in which the subset feature is single valued is still error prone because eSet is called which may also perform inverse operations.
Thanks for the contribution, Thorsten. I'm a little hesitant to modify behavior that has been in place for so long and am surprised that we didn't run into this problem when we initially added support for CDO some 5 years ago now... Christian, do you have any thoughts/insight here?
(In reply to Kenn Hussey from comment #3) > Thanks for the contribution, Thorsten. I'm a little hesitant to modify > behavior that has been in place for so long and am surprised that we didn't > run into this problem when we initially added support for CDO some 5 years > ago now... Christian, do you have any thoughts/insight here? This was so long ago ... I do recall that there were several problems in the handling of the subset/superset constraints, for which we did various patches in UML2 and in CDO's legacy adapter. I do not recall unintended deletion of objects being amongst these problems. Perhaps the testing we did covered only scenarios in which the superset is the containment reference, not cases where the subsets are the containment references? (because the UML Ecore representation has always to choose one or the other). Sorry, not very helpful, but I would be starting out pretty much where the OP is at in investigating this problem.
Thanks, Christian. Thorsten, if you could give me a concrete example that corresponds to the stack trace you provided, it would help me understand better what is happening here. From just looking at the stack trace, it looks like the right thing is happening, i.e., a property is being removed from a containment list and hence its container reference is being unset (and, yes, the object is being orphaned). Unless I'm missing something, it seems that removing the inverse additions/removals when subset/superset constraints are being enforced could lead to other problems (e.g., where containment lists are updated without also updating the corresponding container references)... Note that, technically speaking, the inverseRemove method is being called on a different list from the one originally receiving the basicRemove call, so the API is being respected (for that list).
The problem arises when a given CDOView\CDOTransaction is switched to a different CDOBranchPoint. In that case all objects loaded by the CDOView\CDOTransaction are invalidated and CDOLegacyAdapter(CDOLegacyWrapper).clearEList(InternalEList<?>) line: 968 CDOLegacyAdapter(CDOLegacyWrapper).revisionToInstanceFeature(EStructuralFeature) line: 617 CDOLegacyAdapter(CDOLegacyWrapper).revisionToInstance() line: 494 CDOLegacyAdapter(CDOLegacyWrapper).cdoInternalPostLoad() line: 369 is called. As far as I understand the clearEList call is meant to just remove the objects from the list without any side effect. Therefore notification is turned off and basicRemove method is called in order to avoid that the contained objects are reversely updated in which case they would be detached from the CDOView\CDOTransaction because the eContainer would be set to null. In the "revisionToInstanceFeature" call the instance is later on filled with the objects that are valid at the given CDOBranchPoint. In order to do so basicAdd is called which DOES NOT inversely re-attaches the objects to the eContainer und thus to the CDOView\CDOTransaction. Thus if, in the course of clearEList, an object gets inversely updated by clearing it's eContainer, it stays orphaned even if it is re-added to the list (no matter if it is the super\sub or list itself). This is a very special case related to branching\versioning. But I could imagine that a similar scenario could show up in a collaborative example which also involves invalidation of objects. "Unless I'm missing something, it seems that removing the inverse additions/removals when subset/superset constraints are being enforced could lead to other problems (e.g., where containment lists are updated without also updating the corresponding container references)..." -> It is not required to completely drop inverse handling. This would of course lead to severe problems. It is only required to NOT perform the inverse operation on super\sub in case a basic method is called. In that case the caller should expect exactly that no inverse handling is performed as that is what the API states. "Note that, technically speaking, the inverseRemove method is being called on a different list from the one originally receiving the basicRemove call, so the API is being respected (for that list)." -> I understand that but the point is that it is not the list but the list-object itself which is modified by setting the eContainer to null. And the list- object is by the nature of the SubsetSupersetEObjectEList member of both lists. As a matter of fact the objects inverse is removed (other than stated by the API) the implementation detail why this happens (because remove is called on super) should not matter - I think. Thus I have the feeling that the API is not really respected because not calling basicRemove on super\sub
Hmm, this scenario seems akin to loading a resource, for which there is already a provision in the UML2 list implementation (i.e., the constraint enforcement is guarded by whether the resource is loading). I wonder if it would be possible to to leverage this in the CDO resource implementation such that it is considered "loading" while this invalidation/reloading is taking place?
A branch change can be quite dramatic. If the total change is correctly serialized as normal valid element changes, it should work, but may be incredibly slow as eNotify()'s talk to each other. If the total change is serialized as abnormal element changes, new code paths may be activated. Whether these new code paths are valid may be debatable and/or very hard to accommodate. Unload/Load (or ReLoad) may be simplest. Certainly much simpler than analyzing the total change to establish an equivalent sequence of normal valid element changes.
Yeah, my feeling is that changing the list behavior such that the subset/superset constraints are only partially enforced could lead to inconsistencies/problems that will surface later.
(In reply to Ed Willink from comment #8) > Unload/Load (or ReLoad) may be simplest. Arguably this is also a platform problem and an EMF problem. I find it super-irritating when a GIT change starts an incremental build as soon as the first project changes. This often generates errors that come and go as further projects change and yet more incremental builds churn. I find the idea that a branch change can be micro-incremental very misconceived. The new work is not a continuation of previous work. As a minimum we need a hibernate to suspend all incremental facilities (eNotify() in EMF), then a content only reload, then a refresh to reactivate tracking facilities. NB. EMF loading inhibits eNotify() specifically to avoid thrashing around with inconsistent transient model states while XML loading is incomplete.
