Bug 303466 - CDO not robust when using dynamic packages
Summary: CDO not robust when using dynamic packages
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.core (show other bugs)
Version: 3.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-22 04:35 EST by Caspar D. CLA
Modified: 2010-06-29 09:22 EDT (History)
4 users (show)

See Also:
stepper: review+


Attachments
Patch (18.03 KB, patch)
2010-04-12 05:33 EDT, Caspar D. CLA
no flags Details | Diff
Patch v2 - ready to be committed (18.49 KB, patch)
2010-04-12 06:02 EDT, Eike Stepper CLA
no flags Details | Diff
Testcase (as a patch) (5.65 KB, patch)
2010-04-19 04:22 EDT, Caspar D. CLA
no flags Details | Diff
Patch including testcase, for 3.0 (15.33 KB, patch)
2010-04-22 06:09 EDT, Caspar D. CLA
no flags Details | Diff
Ready to be committed (15.84 KB, patch)
2010-04-27 03:49 EDT, Eike Stepper CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caspar D. CLA 2010-02-22 04:35:00 EST
Using CDO with dynamic packages is very error prone when one goes
beyond the basic scenario of having a single package with
no dependencies.

Once one attempts to work with multiple interdependent packages, 
out-of-the-box there are various problems:

- Ecore XML sent to the server contains file references instead of
  nsURI references, breaking dependency resolution on the server.

- Failure to resolve package dependencies on the client, when packages
  are first used and sent to the server, gives no error message.

- Failure to resolve packages dependencies on the server, when packages
  are used in later transactions, also gives no error message.

One can work around these problems by patching the Ecore files, using 
URI mappings, forcing dependency resolution etc., but it's all rather 
uninviting and very hard to debug when anything goes wrong. For example,
when the client can resolve a package dependency but the server can not,
this goes undetected until there is actual transmission of data, when the
mismatch between the server's reading logic and the client's 
writing logic will (probably) lead to some low-level error.

I believe we should invest in adding some checks to fail early with
clear error messages, and maybe add some mechanisms to relieve the
app coder of having to think a lot about the subtle differences between
generated packages and dynamic packages.
Comment 1 Eike Stepper CLA 2010-02-22 04:53:43 EST
I fully agree.
Comment 2 Stefan Winkler CLA 2010-02-22 11:33:35 EST
Can someone have a look at Bug 302838 and tell me if this could also be the problem there?
Comment 3 Caspar D. CLA 2010-02-22 21:58:44 EST
(In reply to comment #2)
> Can someone have a look at Bug 302838 and tell me if this could also be the
> problem there?

See my comment in bug 302838.
Comment 4 Eike Stepper CLA 2010-03-10 03:20:07 EST
Any news here?
Comment 5 Caspar D. CLA 2010-03-10 04:02:13 EST
(In reply to comment #4)
> Any news here?

Not in the way of solutions for the various symptoms of this issue.

But I have a curious one to add to the list: whether or not the server
adequately processes a set of interdependent dynamic packages when they 
are first committed, seems in certain cases to depend on the order in 
which they are received. This order is not deterministic, probably
due to non-deterministic allocation of the packages in a client-side
HashMap.

Will investigate further.
Comment 6 Caspar D. CLA 2010-04-08 05:49:30 EDT
An update: at long last I've discovered why the server's
ability to correctly process incoming dynamic packages,
depends on the order in which they arrive (which is
non-deterministic).

During construction of EPackage P1's content from the
incoming XML, references to other packages may or may not be
resolvable, depending on whether that referenced package
(P2) was already received and processed or not. This in
itself is not a problem (because resolution is taken care of
later), but when the referenced package is NOT resolvable, a
resource is nevertheless created for it. This resource fails
to load, but is marked as loaded anyway in the ResourceSet
(this seems to be by-design EMF behavior).

Later on, when the XML for P2 is itself processed, CDO
explicitly creates a new resource for it, even though an
empty resource for it may already have been created
"implicitly", as just described. So that gives 2 resources
designated by P2's URI, the 2nd one actually containing the
EPackage content. But the 1st one will get "consulted" when
an reference owned by an element in P1 must be resolved.
This resolution always fails because that 1st resource is
simply empty, but it fails silently (again, apparently this
is by EMF design).

Ultimately the problem manifests itself as communication
problems when CDORevisions are committed, due to the server
having a different idea of, for example, the number of
features that some EClass has, the server having misplaced
features inherited from a base class contained in a
different package.

It took me ages to debug this because every aspect of this
problem goes undetected, i.e. the failure to load the
referenced resource/package, the failure to resolve during
construction of the ePackage, the creation of a second
resource with the same URI, and the failure to resolve at a
later stage. 

I have a patch in place on my own custom code base, which 
seems to fix the problem. Will attempt to rework it for 3.0.
Comment 7 Eike Stepper CLA 2010-04-08 11:59:57 EDT
That indeed sounds scary. I'm looking forward to look at the patch that you mentioned. That will make it clearer to me what really goes on here. Thanks for not giving up!

Do you think Ed should be involved to comment on EMF-related issues?
Comment 8 Caspar D. CLA 2010-04-12 05:33:04 EDT
Created attachment 164530 [details]
Patch

This should fix the problem of duplicate resources getting
implicitly created during the processing of new packages,
with the empty "duds" nevertheless being referenced by some
dependent packages. (The solution is in EMFUtil.createEPackage.)

With this patch in place, things will at least work
correctly if all interdependent packages are transmitted.
There's still no detection of a missing package though..
Will work on that.
Comment 9 Eike Stepper CLA 2010-04-12 06:02:55 EDT
Created attachment 164545 [details]
Patch v2 - ready to be committed
Comment 10 Caspar D. CLA 2010-04-12 06:15:02 EDT
(In reply to comment #9)

Committed to HEAD.

Not resolving bug, because this patch fixed only one of several 
issues.
Comment 11 Caspar D. CLA 2010-04-19 04:22:53 EDT
Created attachment 165245 [details]
Testcase (as a patch)

This patch contains two testcases, one demonstrating how CDO 
does not detect a missing dependency when a new package is 
committed; and the second demonstrating how it does not
detect that the dependent package references the dependency
through a file URI -- which cannot possibly be resolved on the 
server side even if the required package is sent.
Comment 12 Caspar D. CLA 2010-04-20 02:42:58 EDT
Working on a patch to detect missing dependencies. EMF doesn't
seem to provide any tools to perform the proxy resolution in a
way that's quite right for the situation at hand.. having
a discussion with Ed about this, here:

http://www.eclipse.org/forums/index.php?t=tree&th=166582
Comment 13 Caspar D. CLA 2010-04-22 06:09:31 EDT
Created attachment 165723 [details]
Patch including testcase, for 3.0
Comment 14 Eike Stepper CLA 2010-04-27 03:49:07 EDT
Created attachment 166162 [details]
Ready to be committed
Comment 15 Eike Stepper CLA 2010-04-27 03:59:24 EDT
I noticed that your 2 tests do not properly run if they're called from other plugins (like the DBStore tests). Can you somehow use something like OM.BUNDLE.getInputStream(fileName) to load the model? There's an example in org.eclipse.emf.cdo.tests.PackageRegistryTest.loadModel(String). Maybe you want to make this static helper method public and just reuse it?
Comment 16 Caspar D. CLA 2010-04-27 05:20:40 EDT
Tests fixed as you suggested: by making 
PackageRegistryTest.loadModel public and
reusing it.

Committed to HEAD.
Comment 17 Caspar D. CLA 2010-04-27 05:26:55 EDT
Actually I just realized that it would make more 
sense to move the loadModel(String) method to
EMFUtil. Shall I move it?
Comment 18 Eike Stepper CLA 2010-04-27 05:30:34 EDT
Then we would have to pass in the OMBundle instance. Otherwise the common plugin would not find the file in the test plugin. I think that doesn't rectify the effort. But thanks for being sensible ;-)
Comment 19 Eike Stepper CLA 2010-06-29 04:36:12 EDT
Available in 3.0 GA:
http://download.eclipse.org/modeling/emf/cdo/updates/3.0-releases/