Bugzilla – Full Text Bug Listing
| Summary: | CrossReferenceAdapter can cause problems when attached to the RootResource | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Martin Fluegge <martin.fluegge> | ||||||||||||
| Component: | cdo.core | Assignee: | Martin Fluegge <martin.fluegge> | ||||||||||||
| Status: | CLOSED DUPLICATE | QA Contact: | Eike Stepper <stepper> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | martin.fluegge, stepper | ||||||||||||
| Version: | 4.0 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Martin Fluegge
Created attachment 195680 [details]
Test v1
Eike and I were trying to reproduce this problem in a test case but did not succeed yet. But I attach our work so we could continue...
Created attachment 195683 [details]
Patch v1
Our assumption is that the cross reference adapter leads to some sort of recursion when the root resource is attached to the resource set. There are two different approaches to follow:
1.) make sure that adding the root resource to a resource set does not lead to a notification and prevent attaching the CR Adapter to the root resource and its children when the root resource is already attach to the resource set when the CR adapter is added.
2.) do not keep the root resource in the resource set.
This patch tries to implement 1.), but is not yet finished.
O.k. rough idea, which follows the assumtion that new call of setRootResource in getObject might be related to the problem.
This is just conceptual since I am at work an to not even have an eclispe in front of me.
If the created object is a root resource this leads to the call of setRootResource(), which in turn adds the resource to the resource set.
I am not sure about the detailed implementation of the list that holds the resources in the resourceSet, because running through the code is hard while just browsing the svn with a browser. ;)
But, if I have seen correctly the list is not unique, so calling setRootResource() more than once would lead to adding the root resource multiple times, right?
If so (not sure about) shoudn't we take care of this fact in setRootResource() like this?
private synchronized void setRootResource(CDOResourceImpl resource)
{
if(rootResource!=resource)
{
rootResource = resource;
rootResource.setRoot(true);
registerObject(rootResource);
getResourceSet().getResources().add(rootResource);
}
}
I'll check this when I am at home (if I do not find out that my assumtion about the list implementation is incorrect ;) ).
(In reply to comment #3) > But, if I have seen correctly the list is not unique, so calling > setRootResource() more than once would lead to adding the root resource > multiple times, right? Very unlikely. ResourceSet.resources is a bidirectional *containment* list. As such it's implicitely unique. And we seem to have tests that check for the proper size of the resources list. Darn, then let's skip this idea :( Created attachment 195911 [details]
Patch v2
Think I fixed it. Registering and putting the root resource into the object map in getObject seems to lead to the nasty recursion. I excluded it and postponed the addition to the resource set. All test are passing and so are the manual Dawn tests.
Capsar, could you have a look at the tell and say what you think about it?
Created attachment 196002 [details]
UI Test v1
This test makes the problem reproducible. Please run it as SWTBot Test.
I would like to increase the priority of this one from 3 to 2, because if this one is not solved in time it does not make sense to deliver Dawn for Indigo, since it is not useable. Eike, do you agree? (In reply to comment #6) > Created attachment 195911 [details] > Patch v2 > > Think I fixed it. Registering and putting the root resource into the object map > in getObject seems to lead to the nasty recursion. I excluded it and postponed > the addition to the resource set. All test are passing and so are the manual > Dawn tests. > > Capsar, could you have a look at the tell and say what you think about it? I'm not happy with this fix, because it doesn't prevent the bad state (i.e. the rootResource being missing from the view's 'objects' map). Rather, it fixes the state when a caller attempts to retrieve it, and thus pretends that the bad state "never happened" -- but it did. If I changed Bugzilla_337523_Test to retrieve the map with reflection instead of going through the InternalCDOTransaction interface, the assertion in the test would still fail, because the fix logic wouldn't get invoked. > I'm not happy with this fix, because it doesn't prevent the bad state (i.e.
> the rootResource being missing from the view's 'objects' map).
I am confused. My starting point was your test. It passes with the fix and does exactly what the test checks - that the root resource can be retrieved from the objects map. Did you apply patch2. Does your test case really fail?
Btw, why is the test in the attachment annotated to be authored by "Egidijus Vaisnora, Caspar De Groot"? A copy-and-paste mishap? (In reply to comment #10) > > I'm not happy with this fix, because it doesn't prevent the bad state (i.e. > > the rootResource being missing from the view's 'objects' map). > > I am confused. My starting point was your test. It passes with the fix and does > exactly what the test checks - that the root resource can be retrieved from the > objects map. Did you apply patch2. Does your test case really fail? No it does not, because your patch2 effectively corrects the bad state when #getObjects is called. But my point is that this isn't the right approach. You are fixing the bad state when it is about to be detected, instead of preventing the bad state from arising in the first place. I'll update Bugzilla_337523_Test to probe the map using reflection, so as to make my point clear. Created attachment 196088 [details]
Patch for Bugzilla_337523_Test
Bugzilla_337523_Test updated to use reflection. This demonstrates
that "Patch v2" only hides/corrects the problem.. it doesn't prevent
the incorrect state from arising.
Hi Caspar, I do understand that you believe that having the rootResource not in the objects map directly after is creation should be treated as bad state. What I do not understand is why? The only one who is directly aware of the root resource and it storage is AbstractCDOView, so it can handle this fact itself. So I think this is rather an implementation detail. Noone else but the AbstractCDOView access the objects map directly. Accessing the objects field via reflection demonstrates that the root resource is not in, but I guess the no CDO committer will ever retrieve this information by reflection. And I cannot imagine why any CDO user should ever use reflection to get the data. Beside that this would actually be some sort of a hack. The actual problem, and I agree that it is one, only arises from the usage of internal API (calling getObjects()). Any implemented method provided by public API will not run into this problem (e.g. getObject(..)) (as far as I can see). Eike and I worked for several hours on this, but were unfortunately not able to reproduce this in a core test nor to understand every detail of the problem. But we strongly believe that this is not only related to legacy or Dawn, but a core problem. If there is a *must* for putting the root resource into the objects map as soon as possible after its creation, I think it would be better to find another way. Maybe it helps to put the changes from my patch directly to the place where your changes were active. So, simple do not call setResource(), but only register the object and skip the addition to the resource set. Can’t try this from here, but will check this as soon as I am home. If you have any other idea how to solve this I would be thankful for your help :) (In reply to comment #14) > I do understand that you believe that having the rootResource not in the > objects map directly after is creation should be treated as bad state. What I > do not understand is why? The only one who is directly aware of the root > resource and it storage is AbstractCDOView, so it can handle this fact itself. > So I think this is rather an implementation detail. Noone else but the > AbstractCDOView access the objects map directly. I agree. > The actual problem, and I agree that it is one, only arises from the usage of > internal API (calling getObjects()). Any implemented method provided by public > API will not run into this problem (e.g. getObject(..)) (as far as I can see). > Eike and I worked for several hours on this, but were unfortunately not able to > reproduce this in a core test nor to understand every detail of the problem. Both is needed, nonetheless! > But we strongly believe that this is not only related to legacy or Dawn, but a > core problem. Believing is not enough. We need a core test to proof that we've understood. > If you have any other idea how to solve this I would be thankful for your help I'm thinking about removing the laziness from the root resource initialization... (In reply to comment #14) > I do understand that you believe that having the rootResource not in the > objects map directly after is creation should be treated as bad state. No, you misunderstand right there. In general, the rootResource doesn't have to be in the map. Lazy loading is a fine principle. > What I do not understand is why? If you and Eike (just had a long discussion with him about this as well) looked carefully at what Bugzilla_337523_Test tests, then you would see that right before the map is tested for the presence of the root resource, the root resource is * explicitly retrieved by its CDOID * The bug that was reported in zilla 337523, is that the rootResource is still not in the map even after being explicitly loaded! Your fix *undoes* the patch for that, and my *enhanced* (i.e. uploaded today) version of Bugzilla_337523_Test shows that your patch2 for *this* Zilla does not address it. Sorry if I sound repetitive.. but I do get the impression that you and Eike are not carefully looking at what Bugzilla_337523_Test does. Please look again at the code *before* the map is tested. I think I did get that. My only objection relates to your argument that reflective access to implementation details is a (stronger) argument than the use of public API (like, technically, InternalCDOView). (In reply to comment #17) > I think I did get that. My only objection relates to your argument that > reflective access to implementation details is a (stronger) argument than the > use of public API (like, technically, InternalCDOView). I made no such argument. Please forget about the reflective access, because it's only detracting from the point I'm trying to make. The point is this: after retrieving the rootResource explicitly by its CDOID (using view.getObject(rootID)), the rootResource should be present in the view's objects map. Prior to the fix for bug 337523, this was not the case. And after applying Martin's patch for *this* bug, it is again not the case, because this patch undoes the fix for 337523. Martin's patch hides the problem by adding a different check at a different point, which stuffs the rootResource into the map if it's not present. That makes the test pass, but doesn't change the fact that retrieval of the rootResource by its ID fails to add the rootResource to the objects map. And THAT is the original bug 337523. I committed the Dawn UI test (UI test v1) which reproduces the problem. Committed revision 7819 Since we decided to handle this issue by excluding the root resource from the resource set, I mark this one as duplicate of Bug 346636. *** This bug has been marked as a duplicate of bug 346636 *** Committed revision 7840: - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.edit - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.editor Committed revision 7841: - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.emf - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.emf.edit - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.emf.ui - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.gmf - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.gmf.edit - trunk/plugins/org.eclipse.emf.cdo.dawn.codegen.dawngenmodel.gmf.ui Please ignore the last two comments as the were added by mistake. Closing. |