Community
Participate
Working Groups
Related to bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=309305 I got to this problem too. However solution, provided in this issue leads to model inconsistency. Suppose we have users user1 and user2. Both are working on container A. user2 detaches A user1 creates child B in container A user1 commit user2 refresh session user2 commits User2 transaction is committed. Object A is TRANSIENT for user2, but for user1 become INVALID. User1 has element B in CLEAN state, while it's parent is already removed from store. Thus we have garbage in the store. Worst thing that we got to this state silently. IMO expected behavior is to bring A to CONFLICT state and prevent user2 from corrupting model silently. Pascal proposed conflict like solution from the beginning. Why we couldn't go this direction?
Created attachment 198454 [details] Test case v1
When user2 detaches A then this object becomes TRANSIENT and is no longer under the control of the transaction (with the exception that it would be recognized in case of reattachment). The object could even have been attached to a different transaction in the meantime. why should it have any other state than transient?
(In reply to comment #2) Transaction can commit object as detached or it can restore it back to previous state. I would call this control over "server" object, more precisely, control over object removal. I am not certain what will happen, if you move object to different transaction, but I suppose, previous transaction will take care to remove server object by it's CDOID. New transaction, even if it holds reference to the same local object, will introduce new server object with a new CDOID.
(In reply to comment #3) > Transaction can commit object as detached Not exactly. It would commit the parent object which is DIRTY at that time. > or it can restore it back to previous > state. I would call this control over "server" object, more precisely, control > over object removal. I'm not sure what you mean by this. > I am not certain what will happen, if you move object to > different transaction, but I suppose, previous transaction will take care to > remove server object by it's CDOID. It does not matter so much what happens to the object after detachment. Conceptually it seems odd that the object state is stored *within* the objects, while it only has a meaning in the context of a CDOView. But that's probably a different discussion. > New transaction, even if it holds reference > to the same local object, will introduce new server object with a new CDOID. Yes. So, why should an object that has been detached have a CONFLICT?
> So, why should an object that has been detached have a CONFLICT? Because it is a conflict. Attached test case, illustrate typical svn like tree conflict. User2 and User1 made changes to the same object at the same time. While user2 removes object, it must take into account user1 changes, made before last update. Additionally, if you change "expected" value to "false" in the test, you will enable current behavior, where you can see, how user2 successfully commits his changes and leaves orphan "child" in remote repository. User2 was making changes to remote object, not having the whole model locally and worst, it was made silently. How could user2 detect conflict in this case by having current CDO implementation?
(In reply to comment #5) > > So, why should an object that has been detached have a CONFLICT? > > Because it is a conflict. I don't see that. That object will not even be committed anymore. > Attached test case, illustrate typical svn like tree > conflict. CDO does not claim to behave like SVN. > User2 and User1 made changes to the same object at the same time. > While user2 removes object, it must take into account user1 changes, made > before last update. I don't see that. It must take into account changes that others have applied to the parent object. And it does. > Additionally, if you change "expected" value to "false" in the test, you will > enable current behavior, where you can see, how user2 successfully commits his > changes and leaves orphan "child" in remote repository. User2 was making > changes to remote object, not having the whole model locally and worst, it was > made silently. How could user2 detect conflict in this case by having current > CDO implementation? I'll test all this later. Today (day 1 after the release) my workspaces are not very usable, yet.
(In reply to comment #6) > I don't see that. That object will not even be committed anymore. Maybe, I do not understand your position, but it looks like you are claiming that there are no conflict as was no object editing. You don't need to commit object in order to remove. It is enough to send CDOID of removed object. But, regardless how you inform server side about object removal, it is object modification. > CDO does not claim to behave like SVN. Yes, but SVN, like other version control system, deals with concurrent modification and conflict solving. Tree conflict is one of such conflict types and it has nothing to do with SVN as a tool. It is rather common concurrent modification issue. CDO is also solving concurrent modification problems and finds conflicts. Thus tree conflict shouldn't be exclusion and CDO must provide way or strategy how to solve it. However current provided strategy (ignoring) is straight way to model corruption, which IMO should not be acceptable. > I don't see that. It must take into account changes that others have applied to > the parent object. And it does. Perhaps we are not talking about the same situation, but problem is, that object detaching doesn't take into account about newer version on the server. Parent object is handled correctly and it cannot protect concurrent changes made in children level. I think, in the test case this situation is obvious.
Created attachment 198708 [details] TestCase&patch v2 I let my self to develop premature patch. Covers conflict creation on invalidation, when client has TRANSIENT, while from server comes newer version. If we moving to conflict solving based direction, then second part of the patch must be developed. It shuold cover situatuation, when user will attempt to commit TRANSIENT, while on server is newer version. Server must throw exception about newer version.
Created attachment 199008 [details] Demo of the problem with this approach Please look at the second test case I've added.
I have been puzzled with your test case. While moving TRANSIENT object to another transaction, the old transaction still knows current object as detached object, even if the same object is in the state NEW but in different transaction... I think this corner case is open gate to unexpected errors. What if I rollback previous transaction? Silently, object will be resurrected in rollbacked transaction with an old CDOID, while another transaction, which knows object as NEW, will get reference to CLEAN object. How does cross references should behavior on such "migrating" object. Does it mean we can get model graph mixed among different transaction?..
If we follow the same logic as rollback does (see comment 10) and consider that sharing object between transaction is "good", then I need to improve fix, which will ensure to bring object into conflict state (event if it is in different transaction).
You're right, it is a general problem that the CDOState is stored *in* a CDOObject and that the object can have only one state rather than one state per CDOView.
State per view doesn't solves common problem: two transactions will still reference the same EObject and this object will be target for modification from two transactions. What if we solve this corner case following: - if object is TRANSIENT, then ownership of object belongs to transaction, where it was detached (works my patch) - if we attach this object to different transaction, then object is moved to NEW state and this must imply to destroy it's detachment state from previous transaction. Previous transaction needs to loose control over such object. Thus no object will be removed. For this case I will need to make improvement
(In reply to comment #13) > What if we solve this corner case following: > - if object is TRANSIENT, then ownership of object belongs to transaction, > where it was detached (works my patch) > - if we attach this object to different transaction, then object is moved to > NEW state and this must imply to destroy it's detachment state DETACHED? Hasn't it been TRANSIENT? > from previous > transaction. Previous transaction needs to loose control over such object. Would that imply to keep the view pointer for such TRANSIENT objects? > Thus no object will be removed. For this case I will need to make improvement
(In reply to comment #14) > (In reply to comment #13) > > What if we solve this corner case following: > > - if object is TRANSIENT, then ownership of object belongs to transaction, > > where it was detached (works my patch) > > - if we attach this object to different transaction, then object is moved to > > NEW state and this must imply to destroy it's detachment state > > DETACHED? Hasn't it been TRANSIENT? It is synonym. But here detachment state means a little bit more. Transaction still has references to this TRANSIENT object in object list and in save point as detached object. This is object state in transaction which must be destroyed after attaching object to different transaction. If we do not destroy this state, then old transaction will have control over object. > > > from previous > > transaction. Previous transaction needs to loose control over such object. > > Would that imply to keep the view pointer for such TRANSIENT objects? Yes. Information about view is stored in revision, then perhaps some kind of TransientRevision, which shall store weak reference to view. Or maybe some kind of TRANSIENT object registry. Like WeakhashMap, where key-> object, value-> view > > > Thus no object will be removed. For this case I will need to make improvement
(In reply to comment #15) > > Would that imply to keep the view pointer for such TRANSIENT objects? > > Yes. Information about view is stored in revision, then perhaps some kind of > TransientRevision, which shall store weak reference to view. Or maybe some kind > of TRANSIENT object registry. Like WeakhashMap, where key-> object, value-> > view You mean CDOObject, not CDORevision ;-) Why not just use the view pointer in CDOObject? How would legacy objects be dealt with?
(In reply to comment #16) > You mean CDOObject, not CDORevision ;-) Yes > Why not just use the view pointer in CDOObject? Yes, and it should be weak reference > How would legacy objects be dealt with? Legacy elements has CDOLegacyWrapper adapter, which holds ref to CDO view. If legacy element doesn't loose this adapter during detach, when it should work
Created attachment 199418 [details] patch v3 Where were few places, where was checking cdoView == null assuming that it shows transient object. Now seems to be fixed. Both test cases are covered. Left to make server side checking for stale version commit, hold view by weak referene in CDOObject and rework previous patch with idea that TRANSIENT object has reference to transaction Even it is not final patch, feel free to check, if it is going to right direction :)
Created attachment 199781 [details] Patch v3 Fixed AbstractConflictResolver to tolerate objects without ID
Any action pending here?
At current time I stuck with other work, but I think, I will return back to this after ~2 weeks
Committed changes to bugs/350120 branch
Just to be sure, am I supposed to review patch v3 or the tip of bugs/350120?
Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master.
We'll try to address open problems in 4.3 (master) first and then port fixes back to 4.2.
Moving all open bugzillas to 4.5.
Moving all unaddressed bugzillas to 4.6.
Moving all open bugs to 4.7
Moving all unresolved issues to version 4.8-
Moving all unresolved issues to version 4.9
Moving to 4.13.
