[Pivot, UML] Profiles with OCL delegate constraints are leaked forever

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 434554 | | Status | CLOSED FIXED | | Importance | P3 major | | Reported | May 09, 2014 17:32 EDT | | Modified | May 25, 2015 17:17 EDT | | Version | 4.0.0 | | Blocks | 432813 | | Reporter | Christian Damus |

Description

OCL Luna M7

In investigation of Papyrus bug 432813 (in which every UML model having profiles applied is leaked forever if it has been validated and the profiles have OCL constraints using the validation delegates), I have tracked down a memory leak into the OCL Pivot environment's domain/metamodel-manager adapter management.

Papyrus is paranoid about always unloading all resources from a resource set when closing the editor, because otherwise all UML content is leaked in the UML CacheAdapter. However, If a model was validated that has OCL constraints in an applied profile, the OCLDelegateDomain from the pivot environment that is attached to the dynamic EPackage definition of the profile is retained in the global EValidator registry. So, even after closing the model editor, this EPackage retains the entire UML profile that was loaded in the editor, because the EPackage is contained by the Profile.

The reason for this is that the DelegateResourceAdapter attached to the UML profile resource (being the resource that contains the EPackage, indirectly via the profile) doesn't account for the fact that the EPackage was not contained directly by the resource when the resource is unloaded (see DelegateResourceAdapter::unloadDelegate() method). So, it doesn't unload the EPackage's delegates, and the OCLDelegateDomain, MetaModelManager, etc. are leaked.

[eclipse-ocl-bot]

By Christian Damus on May 09, 2014 17:40

I have published a Gerrit change that fixes the problem and provides a new JUnit test that fails without my proposed fix:

https://git.eclipse.org/r/26315

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 02:06

See also Bug 434451.

Please don't waste your time and my time with Gerrit; it just doesn't work. I uploaded youir change set twice but can't find where they've gone to so I end up pasting changed lines. Really naff. GIT Patches work.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 03:03

(In reply to Christian W. Damus from comment #0)

The reason for this is that the DelegateResourceAdapter attached to the UML profile resource (being the resource that contains the EPackage, indirectly via the profile) doesn't account for the fact that the EPackage was not contained directly by the resource when the resource is unloaded (see DelegateResourceAdapter::unloadDelegate() method). So, it doesn't unload the EPackage's delegates, and the OCLDelegateDomain, MetaModelManager, etc. are leaked.

Yes. I usually find MetaModelManager leakage to be a very good leakage metric; hence the ability to set liveMetaModelManagers for diagnosis, which in conjunction with PivotTestCase.DEBUG_GC enables me to verify occasionally thart the entire test suite leaks no MetaModelManagers. Running the tests with a limited -Xmx provides some regular checks against major leakage.

Your analysis and fix look plausible, but the test doesn't work for me.

Achieving zero leakage is very hard because the SynchronizedXtextResourceSet tends to hang on to a Resource so a MetaModelManager so everything.

Your test still has a non-null metaModelManager stack variable so I don't see how it could have worked.

I'm not clear whether the complexity of collectGarbage() is necessary. Surely System.gc() and System.runFinalization() is good enough.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 03:59

(In reply to Ed Willink from comment #3)

Your analysis and fix look plausible, but the test doesn't work for me.

Running your test (less the GC) with and without your fix and with the PivotTestCase DEBUG_GC help makes no difference. I get no leaked MetaModelManagers, pivot ProfileImpl or *DelegateDomain in either case. There are consistently three residual EPackageImpl's presumably from static packages.

The secret to no leakage with the Pivot is to make sure that MetaModelManager.dispose() gets called, possibly via OCL.dispose(). Many objects implement MetaModelManagerListener to make sure that they can clean up.

Unfortunately models have an incredibly complex web of internal references so it can be very difficult to spot what is inhibiting GC. I suspect that you have picked the wrong culprit to cure. Perhaps Bug 434451 was the problem.

You might want to look at PivotTestCase.GlobalStateMemento adapted from xtext.junit as an aid to more debuggable global registries.

Or you could just add to some init code:

MetaModelManager.liveMetaModelManagers = new WeakHashMap<MetaModelManager,Object>(); // Prints the create/finalize of each MetaModelManager

so that you see how many live MetaModelManagers you have.

Bug 434564 raised to avoid the thrashing of MetaModelManagers that you will observe.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 07:25

For the particular use case of total clean up at the end of a Papyrus session, I don't see the need for this fix. OCLDelegateDomain implements MetaModelManagerListener and so MetaModelManager.dispose() calls OCLDelegateDomain.metaModelManagerDisposed() which calls OCLDelegateDomain.reset() in just the same way that DelegateEPackageAdapter.unloadDelegates() does. The true problem for this use case must be MetaModelManager leakage, for which Bug 434451 fixes one cause.

The extra unloadDelegates() does no harm to existing tests, but I'm worried that it may introduce a problem in the following scenario that might arise from e.g. Profile re-definition/re-application.

App uses dynamic EPackages P1 and P2 both of which have delegates and so the delegataes for P1 and P2 share the OCL/MetaModelManager context for their common ResourceSet. Replacing P1 by P1a, while retaining P2 currently does very little (apart perhaps from leaking). With your extra unloadDelegates() the shared OCL/MetaModelManager context for P1+P2 gets disposed. P1a will create another one but I'm not sure that P2 will be happy.

So I'm inclined to WONTFIX at least for now and locate the MetaModelManager leakage.

(A known client count in OCL might be necessary to ensure that OCL.dispose() only takes effect for the last client.)

[eclipse-ocl-bot]

By Christian Damus on May 10, 2014 07:35

(In reply to Ed Willink from comment #2)

See also Bug 434451.

Please don't waste your time and my time with Gerrit; it just doesn't work. I uploaded youir change set twice but can't find where they've gone to so I end up pasting changed lines. Really naff. GIT Patches work.

Oh ... will you remove the OCL Gerrit instance, then?

(In reply to Ed Willink from comment #3)\

Your analysis and fix look plausible, but the test doesn't work for me.

Interesting. I'll see whether I can fix that and post a Git patch. At the very least, my patch in the package adapter is verifiable with profiling tools such as YourKit as fixing the leak scenario in Papyrus.

Achieving zero leakage is very hard because the SynchronizedXtextResourceSet tends to hang on to a Resource so a MetaModelManager so everything.

I haven't yet looked into what happens when the user edits OCL expressions. So far, I'm trying to keep users' workbenches from running out of memory when they simply open models, validate them, and close them again. :-/

Your test still has a non-null metaModelManager stack variable so I don't see how it could have worked.

Because the MetaModelManager clears references to the EPackage and its contents, I suppose. It worked for me. It could be difficult for me to determine why it doesn't for you ... Perhaps it's a problem when running with the rest of the suite.

I'm not clear whether the complexity of collectGarbage() is necessary. Surely System.gc() and System.runFinalization() is good enough.

System.gc() is not guaranteed to do anything when requested; it's specified as a hint. Historically, VMs have varied widely in its implementation. The idiom in that patch is fairly common in these sorts of tests.

(In reply to Ed Willink from comment #4)

The problem for me is that (a) the code in Papyrus that triggers the leak has no dependencies on the OCL API (the OCL delegates etc. are plugged in via extension points) and (b) there doesn't seem to be a clean way to find the MetaModelManagers and/or OCL facades from the context of a model, to clean them up, even if the API dependencies were available.

Besides, it is clear that the OCL machinery goes out of its way with a Resource adapter to try to unload itself when an EPackage is unloaded, and indeed this works for EPackages that are directly contained in *.ecore resources, it just doesn't work for those that are nested within UML Profiles. So, I plug that little hole. :-)

[eclipse-ocl-bot]

By Christian Damus on May 10, 2014 07:42

(In reply to Ed Willink from comment #5)\

App uses dynamic EPackages P1 and P2 both of which have delegates and so the delegataes for P1 and P2 share the OCL/MetaModelManager context for their common ResourceSet. Replacing P1 by P1a, while retaining P2 currently does very little (apart perhaps from leaking). With your extra unloadDelegates() the shared OCL/MetaModelManager context for P1+P2 gets disposed. P1a will create another one but I'm not sure that P2 will be happy.

It seemed to me that each EPackage has its own delegate domain ... is that not the case? Either way, defining a profile again only adds a new EPackage. If the profile resource is unloaded, then all EPackages within it become proxies and are no longer useful, so the domain(s) need(s) to be unloaded anyways. If a *.ecore resource in the resource set is unloaded, the current M7 code will unload just as my patch does. What's the difference?

So I'm inclined to WONTFIX at least for now and locate the MetaModelManager leakage.

If you can fix the leak another way, that's certainly fine by me! :-)

[eclipse-ocl-bot]

By Christian Damus on May 10, 2014 07:44

(In reply to Ed Willink from comment #5)

DelegateEPackageAdapter.unloadDelegates() does. The true problem for this use case must be MetaModelManager leakage, for which Bug 434451 fixes one cause.

That's a hot code replace bug in JDT.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 08:23

(In reply to Christian W. Damus from comment #8)

Bug 434451 fixes one cause.

Bug 434541

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 08:32

(In reply to Christian W. Damus from comment #7)

It seemed to me that each EPackage has its own delegate domain ... is that not the case?

Yes but not necessarily its own OCL/MetaModelManager.

The problem comes from Complete OCL, which through its ability to extend OclAny or Boolean means that each user OCL usage might (but almost certainly doesn't) have a distinct type system. Creating a new MetaModelmanager for each type system is an undesirable cost so I try to re-use them; mytations endeavourt to create a mutable copy for the mutation.

For OCL added as EMF delegates, Complete OCL extensions are not possible, so such delegates share the single global OCL context, which is yet another one-off 'leak'.

For OCL added to dynamic EPackages the containing ResourceSet identifies a logical owner/extent/domain so we can share the type system via MemaModelManagerResourceSetAdapter.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 11:43

(In reply to Christian W. Damus from comment #6)

my patch in the package adapter is verifiable with profiling tools such as YourKit as fixing the leak scenario in Papyrus.

Your fix is definitely good for the validate and close editor scenario. The MetaModelManager was created for the OCLDelegateDomain's OCL. The fix releases this 'ownership'.

commit ca7e378c28e0732cc635bf6980901064dc60bfa1 pushed to master for RC1.

(excluding the test).

(In reply to Christian W. Damus from comment #6)

Your test still has a non-null metaModelManager stack variable so I don't see how it could have worked.

Because the MetaModelManager clears references to the EPackage and its contents, I suppose. It worked for me. It could be difficult for me to determine why it doesn't for you ... Perhaps it's a problem when running with the rest of the suite.

I can only assume that you attached the wrong version. I needed to null at least ocl/metaModelManager/umlResource and still it didn't work.

It would be nice to have a good leakage test, but the diverse global registries make this very hard. All the hammers in PivotTestCase are needed to break references and the GlobalStateMemento to avoid registrations getting lost for further tests.

Maybe an overall Papyrus open, diverse activity, close, no leakage test will do.

[eclipse-ocl-bot]

By Ed Willink on May 10, 2014 12:18

I can only assume that you attached the wrong version. I needed to null at least ocl/metaModelManager/umlResource and still it didn't work.

I cannot locally avoid XMLHandler classLoader statics referring through the UMl cache adapter to the ProfileApplicationImpl, that references the ProfileImpl.

I see little prsopect of getting a UML content test working without significant interaction and subsequent restoration of the gloabl registruies, so this will have do go without a test.

[eclipse-ocl-bot]

By Christian Damus on May 11, 2014 08:11

Thanks for your help, Ed! I do have an integration-style test in the Papyrus test suite waiting to be activated when we pick up an OCL build that has this fix. So, that provides some of the coverage you're looking for (though it won't run in the OCL project's builds, of course).

[eclipse-ocl-bot]

By Ed Willink on May 12, 2014 02:53

(In reply to Christian W. Damus from comment #13)

Thanks for your help, Ed! I do have an integration-style test in the Papyrus test suite waiting to be activated when we pick up an OCL build that has this fix. So, that provides some of the coverage you're looking for (though it won't run in the OCL project's builds, of course).

I have an additional consumers-tests Hudson Job to which I could add something similar. So far it checks SysML parsing, which was a disaster in Kepler.

Can you post the test identity and maybe I'll get round to creating a variant of it.

[eclipse-ocl-bot]

By Christian Damus on May 12, 2014 09:04

https://hudson.eclipse.org/hudson/view/Modeling/job/papyrus-trunk-nightly-tests/lastCompletedBuild/testReport/org.eclipse.papyrus.tests.AllTests$org.eclipse.papyrus.editor.integration.tests.AllTests$org.eclipse.papyrus.editor.integration.tests.tests/EditorMemoryLeakTest/testValidatedProfiledModelContentDoesNotLeak/

https://git.eclipse.org/c/papyrus/org.eclipse.papyrus.git/tree/tests/junit/plugins/core/org.eclipse.papyrus.editor.integration.tests/src/org/eclipse/papyrus/editor/integration/tests/tests/EditorMemoryLeakTest.java

[eclipse-ocl-bot]

By Christian Damus on May 16, 2014 07:11

Will this fix be published in an integration build? The last I-build is from 1 May, which was before M7.

[eclipse-ocl-bot]

By Ed Willink on May 16, 2014 07:50

(In reply to Christian W. Damus from comment #16)

Will this fix be published in an integration build? The last I-build is from 1 May, which was before M7.

There already is an N-build.

I-build underway.

[eclipse-ocl-bot]

By Ed Willink on May 16, 2014 08:07

(In reply to Ed Willink from comment #17)

I-build underway.

Could be some time. Looks like there is/was a problem with a corrupt

http://download.eclipse.org/eclipse/updates/4.4-I-builds

[eclipse-ocl-bot]

By Christian Damus on May 16, 2014 08:10

Thanks, Ed. I'm old-fashioned: prefer I-builds for build dependencies :-) Not meaning to press ... I am happy to wait, just didn't know how the schedule works.

[eclipse-ocl-bot]

By Ed Willink on May 16, 2014 09:47

(In reply to Ed Willink from comment #18)

(In reply to Ed Willink from comment #17)

I-build underway.

Could be some time.

Build has now completed. Successful builds can auto-priomote these days, so its already on the downloads page.

(N-builds are triggered by GIT polling. I-builds are 'triggered;' when I think there is some significant progress. Branch-builds protect you against the bleedingest edge.)

(In reply to Christian W. Damus from comment #19)

Thanks, Ed. I'm old-fashioned: prefer I-builds for build dependencies :-) Not meaning to press ... I am happy to wait, just didn't know how the schedule works.

We have\ N-builds dependent on N/I/S/R\ I-builds dependent on I/S/R\ S-builds dependent on S/R

Dependencies on N-builds are very important to get an early detection of over-enthusiastic evolution by other projects. If you leave it till two days before a milestone, it can be too late for a rational resolution of a problem.

[eclipse-ocl-bot]

By Christian Damus on May 16, 2014 18:40

Verified in today's Papyrus JUnit test build. Thanks!

[eclipse-ocl-bot]

By Ed Willink on May 25, 2015 17:17

CLOSED after more than a year in the RESOLVED state.