Bugzilla – Full Text Bug Listing
| Summary: | Detach-reattach of dirty object discards pre-detach featureDeltas | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Cyril Jaquier <cyril.jaquier> | ||||||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | caspar_d, saulius.tvarijonas | ||||||||||||||
| Version: | 4.0 | Flags: | stepper:
review+
|
||||||||||||||
| Target Milestone: | --- | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Whiteboard: | offline-xx | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Cyril Jaquier
Created attachment 174283 [details]
Test Case v1
Using the attached test case, here is the output WITHOUT "remove" before "add" (looks good): Delta revision(s): NodeA@OID5:0v1 CDOFeatureDelta[null, CONTAINER, resource=NULL, container=OID6, feature=-1] NodeA@OID6:0v1 CDOFeatureDelta[null, CONTAINER, resource=NULL, container=OID2, feature=-1] CDOListFeatureDelta CDOFeatureDelta[children, ADD, value=OID5, index=0] NodeA@OID2:0v1 CDOListFeatureDelta CDOFeatureDelta[children, ADD, value=OID6, index=0] NodeA@OID3:0v1 CDOListFeatureDelta CDOFeatureDelta[children, REMOVE, value=UNKNOWN, index=2] CDOFeatureDelta[children, REMOVE, value=UNKNOWN, index=1] And now the output WITH "remove" before "add" (looks not so good): Delta revision(s): NodeA@OID6:0v1 CDOListFeatureDelta CDOFeatureDelta[children, REMOVE, value=UNKNOWN, index=2] CDOFeatureDelta[children, REMOVE, value=UNKNOWN, index=1] NodeA@OID3:0v1 CDOFeatureDelta[null, CONTAINER, resource=NULL, container=OID5, feature=-1] NodeA@OID5:0v1 CDOListFeatureDelta CDOFeatureDelta[children, ADD, value=OID3, index=0] NodeA@OID2:0v1 CDOFeatureDelta[null, CONTAINER, resource=NULL, container=OID3, feature=-1] An ADD feature delta is missing. It has been generated but removed later from CDOSavepointImpl. Created attachment 174285 [details]
Test Case v2
I added a variation of the first test case which duplicate nodes. It is the same bug but with a different consequence ;-)
Created attachment 174286 [details]
Test Case v3
Sorry, a new (and hopefully the last) variation on the same theme (bug) again. This one results in a cycle in the tree which get materialized as a StackOverflowError in our application (followed by a database cleanup...)
Cyril, please confirm that: 1) The number of lines that you changed is smaller than 250. 2) You are the only author of these changed lines. 3) You apply the EPL to these changed lines. After some tests I feel that the problem is related to re-attachment (possibly in conjunction with detachment). Assgining to Jasper... Created attachment 174378 [details]
Small fix to CDOSavepointImpl - does not fix the bug!
Created attachment 174381 [details]
Test Case v4
Slightly modified version of the test case to conform to the following points:
1) The number of lines that you changed is smaller than 250.
Yes
2) You are the only author of these changed lines.
Yes
3) You apply the EPL to these changed lines.
Yes
Comment on attachment 174381 [details]
Test Case v4
Committed to HEAD
Proposed patch 'Small fix to CDOSavepointImpl' breaks rollback logic where reattached objects are concerned. As discussed on Skype, CDOTransactionImpl.rollback needs objects that were detached-then-reattached within the same savePoint, to remain present in detachedObjects, so that it can identify these special cases and treat them appropriately. About the problem itself, it is indeed related to the re-attachment logic, in that the reattachment support creates the opportunity for this bug to arise. What happens is that the normal detachment logic (which predates the reattachment feature) assumes that when an object is detached, the revision delta associated with it, can be discarded. This happens in CDOSavePointImpl.java, line 73: revisionDeltas.remove(key); Oops. If that revDelta contained any featureDeltas, then those have been lost. If the object is later reattached, then the changes that were made to that object prior to its detachment, are, as a consequence, missing from the TX. In the testcase provided therefore, the removal of the n3 child from n2, is lost from the TX when n2 itself is detached (although locally the change is still known.) And so n2 still has n3 as its child when it's view in a different session. Good find, Cyril. I don't have time to fix it right now, but will do so early next week. I have found out the same thing in the meantime. My first attempt to fix it was to store the removed revision delta in a new map. Only to find out then that the logic in the ReattachTransition computes this delta anyway from comparing two states. I now suspect that not the right states are used to do the comparison. BTW. I have the feeling that other issues may arise from the formerRevisions map being stored in the transaction and not in the savepoints. Should we fix that, too, Caspar? (In reply to comment #12) > I now suspect that not the right states are used to do the > comparison. Yeah, the comparison is done between the newly constructed revision, the values of which are populated from the CDOObject itself, and the revision that was backing the object at the moment of detachment. Obviously this fails to re-create the deltas that had affected the revision prior to detachment. > BTW. I have the feeling that other issues may arise from > the formerRevisions map being stored in the transaction > and not in the savepoints. Should we fix that, too, > Caspar? Well... let's see. The immediate issue is the restoration of the deltas. Either we keep them somewhere, as you were trying to do, or we re-create them. The latter requires as a basis for comparison, the revision that backed the object in its CLEAN state. So that is yet another reason why we need the session to somehow hold on to the revisions that it originally loaded (other reasons being bug 315026, bug 312534, and bug 312535). And if the session keeps these "clean revisions" somewhere, I don't think we'll need the formerRevisions map anymore. I might go ahead and attempt to fix it this way... although I'm still wondering about how we're going to make this work with rollbacks-to-savepoints, passiveUpdates, and refreshes. (That is: what is a "CLEAN" revision anyway? The one that was originally loaded? Or the one that represents the last known server state? Each answer has problematic implications for the rollback and update features...) (In reply to comment #13) > > I now suspect that not the right states are used to do the > > comparison. > > Yeah, the comparison is done between the newly constructed > revision, the values of which are populated from the > CDOObject itself, and the revision that was backing the > object at the moment of detachment. Obviously this fails to > re-create the deltas that had affected the revision prior to > detachment. Can you fix this short term (as opposed to the other issues you mentin below)? > > BTW. I have the feeling that other issues may arise from > > the formerRevisions map being stored in the transaction > > and not in the savepoints. Should we fix that, too, > > Caspar? > > Well... let's see. The immediate issue is the restoration of > the deltas. Either we keep them somewhere, as you were > trying to do, or we re-create them. The latter requires as a > basis for comparison, the revision that backed the object in > its CLEAN state. So that is yet another reason why we need > the session to somehow hold on to the revisions that it > originally loaded (other reasons being bug 315026, bug > 312534, and bug 312535). And if the session keeps these > "clean revisions" somewhere, I don't think we'll need the > formerRevisions map anymore. Would that include the ability to rollback to arbitrary savepoints? Or only to the first (implicit) one? I have the intuition that each save point should store what *it* thinks the "original" revision was. > I might go ahead and attempt to fix it this way... although > I'm still wondering about how we're going to make this work > with rollbacks-to-savepoints, passiveUpdates, and refreshes. > (That is: what is a "CLEAN" revision anyway? The one that > was originally loaded? Or the one that represents the last > known server state? Each answer has problematic implications > for the rollback and update features...) I agree that with passiveUpdates==true some subtle issues may arise, but in general I think that's accepted by users of this mode, otherwise they should not use it. Of course we can discuss if we know of better overall approaches to passiveUpdates but I think that should not be done in this bugzilla ;-) (In reply to comment #14) > Can you fix this short term (as opposed to the other > issues you mentin below)? Will try. > <SNIP> > Would that include the ability to rollback to arbitrary > savepoints? Or only to the first (implicit) one? I have > the intuition that each save point should store what *it* > thinks the "original" revision was. What's the reason behind this intuition? As far as I can oversee now, in all cases where the clean revision is needed (see list of bugs above), it is irrelevant in which savePoint the object first became dirty. And having a separate cleanRevisions collection for each savePoint, would introduce a significant computational burden, because a clean revision would have to be looked up in the union of all these separate collections. So if we have no clear reason for this approach, I don't think it's a good idea. But I might be overlooking sth... BTW, in general the idea of a set of "clean revisions" is a bit wobbly, since revisions aren't immutable :-( But I guess we'll have to live with that; or alternatively, we can introduce sth like a #freeze() method which makes the revision immutable from then on. (This does make me wonder a bit whether it was such a good idea to eliminate #isTransactional. It would have come in handy, since isTransactional == !isImmutable). > <SNIP> > I agree that with passiveUpdates==true some subtle issues > may arise, but in general I think that's accepted by users > of this mode, otherwise they should not use it. Of course > we can discuss if we know of better overall approaches to > passiveUpdates but I think that should not be done in this > bugzilla ;-) I doubt that the issues are "subtle" and "accepted", but I agree we should have a separate Bugzilla for that discussion. Will open shortly. Created attachment 174703 [details]
Patch
Thank you Caspar. Looks good and can be committed. Changed bug summary. Was: "Moving nodes in a tree structure may result in an inconsistent tree" Committed to HEAD (In reply to comment #19) > Committed to HEAD Thank you Caspar :-) Just tested your patch in our application and I was not able to reproduce this bug anymore. *** Bug 321086 has been marked as a duplicate of this bug. *** Available in R20110608-1407 |