Bugzilla – Full Text Bug Listing
| Summary: | Notify clients about changes in the dangling state of objects | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||||
| Status: | NEW --- | QA Contact: | Eike Stepper <stepper> | ||||||||||||
| Severity: | enhancement | ||||||||||||||
| Priority: | P3 | CC: | mathieu.velten, pascal.lehmann, saulius.tvarijonas, stepper | ||||||||||||
| Version: | 4.13 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Caspar D.
Created attachment 189926 [details]
Testcase
We ran into a similar problem: - Locally object B references A (not yet committed) - Remotely object A references B. When doing a refresh (or getting the invalidation), our eclipse view displaying the references ended up having a StackOverflowError because of the introduced cycle. It should not be possible to commit this faulty data since we have XRef checking, but it would be nice to have a handler called (eg. something like a ConflictResolver whenever the transaction to be updated is dirty), to take care of such situations before any notifications are sent out. (In reply to comment #2) > We ran into a similar problem: > - Locally object B references A (not yet committed) > - Remotely object A references B. > > When doing a refresh (or getting the invalidation), our eclipse view displaying > the references ended up having a StackOverflowError because of the introduced > cycle. > It should not be possible to commit this faulty data since we have XRef > checking, but it would be nice to have a handler called (eg. something like a > ConflictResolver whenever the transaction to be updated is dirty), to take care > of such situations before any notifications are sent out. I'm not sure why you would consider this a flaw in CDO. In your description of the problem you do not state that it is illegal (on an EMF-model level) for A to reference B and B to reference A at the same time. As for my own problem, I'm not yet sure how to deal with it. Since the other commit has already succeeded, the incoming reference cannot be ignored. Rather, it seems that the new reference must be added (i.e the remote delta must be applied), and then removed again locally, which would add a remove delta to the tx. But is this always the desired behavior? Or should it be pluggable? Another consideration: I think this problem isn't limited to PU=false (i.e. refreshes). It seems the same problem can occur with PU=true, when a CommitNotification comes in. Created attachment 190018 [details]
Patch (excluding testcase)
This shows how/where to detect the problem, but doesn't actually
deal with it yet.
I may be dumb but I can't see the actual problem here. Sure, your test case fails on the second assertNull(detail.getProduct()), but I'm sure that you just must not expect null at that time. How does that situation differ from a one-client-only scenario: res.getContents().add(x); res.getContents().add(y); tx.commit(); res.getContents().remove(x); y.setRef(x); // Why should this not be allowed? ref= y.getRef()); // Why should ref suddenly be null? tx.commit(); // This must throw a DanglingReferenceException The difference between the example you give, and the situation that I consider a problem, is this: in your example, the client knowingly causes the situation; in the problem scenario, the client has no way of either knowing about the situation, nor of preventing it. So yes, you're right to say that a dangling ref isn't "wrong" in itself but I consider it wrong that they are silently introduced by a refresh. Created attachment 190027 [details]
Testcase v2 (as a patch)
We (Eike and I) sort-of agreed on Skype that this isn't quite a bug, but rather
an enhancement request asking for some kind of notification mechanism that
would allow a client app to be made aware of the arising of dangling references
due to incoming refresh results or commitNotifications. Bug title was amended
accordingly.
As you suggested, I looked at the existing mechanism sending
CDOViewInvalidationEvents, which, as I've come to realize, already affords a
way of detecting this scenario. Hence, I'm not convinced anymore any change is
necessary at all. Will get back on this.
The patch I'm uploading demonstrates how an IListener implementation can detect
the deltas that cause dangling refs, and fix them.
Sorry, I should have been more precise when describing our problem. We use the references to 'model' a second hierarchy (the first one is already done using containments). So the problem with the cycle is that both nodes are each others parent and child at the same time - which should not be possible. Therefore something needs to be 'resolved' before the notification are sent out further. The problem is not limited to the reference, it also happens to the containment itself, though afaik containment aren't checked during the XRef check on the server and therefore it might be possible to damage the model (build a containment cycle) on the server. (In reply to comment #9) > Sorry, I should have been more precise when describing our problem. We use the > references to 'model' a second hierarchy (the first one is already done using > containments). So the problem with the cycle is that both nodes are each others > parent and child at the same time - which should not be possible. But these are your business semantics, right? As far as EMF/CDO is concerned, these reciprocal references are not "model breakage". I therefore do still consider this different from what I initially reported. You are trying to enforce your business semantics, while I am concerned with the fact that CDO silently creates dangling references (which are, IMO, a mild form of breakage at the EMF/CDO level.) > Therefore something needs to be 'resolved' before the notification are sent > out further. For my original problem, the CDOViewInvalidationEvents might already be an adequate interception mechanism to deal with this. Perhaps you can use them too? I think it's similar as there should be a call some kind of 'conflict resolver' code whenever a refresh or invalidation arrives at a dirty transaction (even when there are no conflicts at all), as applying the changes might break the consistency (in your case a dangling ref, in our a cycle in a tree structure).
Cycles in references are fine (even though it would be nice if we could intercept this too since we treat it as if it was a containment to model a second tree structure) whereas in containments they are not.
I'll post a testcase showing the problem, but I'm rater busy this week so it will probably be next week.
> For my original problem, the CDOViewInvalidationEvents might already be an
> adequate interception mechanism to deal with this. Perhaps you can use
> them too?
Thanks, I'll have a look :)
(In reply to comment #11) > [...] CDO silently creates dangling references (which are, IMO, > a mild form of breakage at the EMF/CDO level.) Just that we don't confuse other users/readers: Whatever we call this, dangling references are perfectly permitted in local models and do not harm the business portion of a model. They just may (can) not be persisted and CDO perfectly recognizes and refuses them on tx.commit(). I know that you really just want to get aware of this situation earlier ;-) Created attachment 190276 [details]
Containment Cycle Testcase v1
Ok, I was able to throw together a small testcase to show the problem.
It produces a cycle in a tree without a conflict (ConflictResolver would never be called, though I don't have one attached in this example).
The testcase usually leads to the following exception:
Exception in thread "InvalidationRunner-Transaction 1" java.lang.StackOverflowError
at java.util.concurrent.ConcurrentHashMap.get(Unknown Source)
at org.eclipse.net4j.internal.util.om.LegacyPlatform.getDebugOption(LegacyPlatform.java:44)
at org.eclipse.net4j.internal.util.bundle.AbstractBundle.getDebugOption(AbstractBundle.java:171)
at org.eclipse.net4j.internal.util.bundle.AbstractBundle.getDebugOption(AbstractBundle.java:142)
at org.eclipse.net4j.util.om.trace.Tracer.isEnabled(Tracer.java:82)
at org.eclipse.net4j.util.om.trace.ContextTracer.isEnabled(ContextTracer.java:57)
at org.eclipse.emf.internal.cdo.view.CDOStateMachine.read(CDOStateMachine.java:298)
at org.eclipse.emf.internal.cdo.view.CDOStoreImpl.getRevisionForReading(CDOStoreImpl.java:678)
at org.eclipse.emf.internal.cdo.view.CDOStoreImpl.isSet(CDOStoreImpl.java:208)
at org.eclipse.emf.internal.cdo.CDOObjectImpl.eDynamicIsSet(CDOObjectImpl.java:555)
at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eIsSet(BasicEObjectImpl.java:1263)
at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eIsSet(BasicEObjectImpl.java:1247)
at org.eclipse.emf.ecore.util.EContentsEList$FeatureIteratorImpl.hasNext(EContentsEList.java:407)
at org.eclipse.emf.ecore.util.EContentAdapter.setTarget(EContentAdapter.java:222)
at org.eclipse.emf.ecore.util.EContentAdapter.setTarget(EContentAdapter.java:188)
at org.eclipse.emf.common.notify.impl.BasicNotifierImpl$EAdapterList.didAdd(BasicNotifierImpl.java:127)
at org.eclipse.emf.internal.cdo.CDOObjectImpl$1.didAdd(CDOObjectImpl.java:424)
at org.eclipse.emf.internal.cdo.CDOObjectImpl$1.didAdd(CDOObjectImpl.java:1)
at org.eclipse.emf.common.util.BasicEList.addUnique(BasicEList.java:425)
at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:307)
at org.eclipse.emf.common.notify.impl.BasicNotifierImpl$EAdapterList.add(BasicNotifierImpl.java:199)
at org.eclipse.emf.ecore.util.EContentAdapter.addAdapter(EContentAdapter.java:352)
at org.eclipse.emf.ecore.util.EContentAdapter.setTarget(EContentAdapter.java:225)
at org.eclipse.emf.ecore.util.EContentAdapter.setTarget(EContentAdapter.java:188)
If the test does not get stuck there (it doesn't seem to happen every time), the cycle is committed without an error leading to the objects still being contained (eg. not flagged as deleted) but no longer referenced from the tree root in the DB. This can have some unpredictable effects when the client still has references to those cycled objects.
I haven't looked into intercepting the cycle by adding an IListener to the transaction, will do later.
Created attachment 190281 [details]
Containment Cycle Testcase v2
I accidentially removed too much output from the Testcase leaving an infinte loop :p replaced with v2
Moving all open enhancement requests to 4.1 Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Moving all outstanding enhancements to 4.3 Moving all open enhancement requests to 4.4 Moving all open enhancement requests to 4.4 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. |