Open eclipse-uml2-bot opened 1 week ago
eclipse-uml2-bot
commented
1 week ago By Kenn Hussey on Jan 11, 2017 21:17
Hmm, I'm not sure of a safe way to address this other than to overload the method with an alternative that doesn't clear adapters and have Papyrus call that instead. The existing behavior has been in place for many years and it would be next to impossible to predict which downstream clients might depend on it behaving this way...
eclipse-uml2-bot
commented
1 week ago By Thorsten Schlathölter on Jan 12, 2017 04:50
I understand. But this is not a papyrus issue. It's an issue of CDO and UML. When this method is used with CDO persistence the object will never get destroyed because of an Exception. Destroying an object in such a way generally predicts that there is no adapter ever that may be needed at the destroyed object. There are other potential issues lurking here.
I think it would be a good idea to document this potential pitfall and to provide an alternative method that does not remove the adapters.
Papyrus and other applications could then decide wether to stay with the old implementation or migrate to the new one.
eclipse-uml2-bot
commented
1 week ago By Ed Willink on Jan 12, 2017 08:23
(In reply to Thorsten Schlathölter from comment #2)
I think it would be a good idea to document this potential pitfall
I don't think that is necessary.
UML2Util.destroy() seems self explanatory. The object is destroyed, and of course no longer useable. Therefore any caller that expects ongoing use should not be calling it.
and to provide an alternative method that does not remove the adapters.
A new UML2Util.detach()/unload() might be UML-only, but where is the boundary? If OCL is used as an enrichment of UML then should OCL adapters be removed too? How does UML know what is a simple rather than complex UML enrichment?
Perhaps all UML adapters might implement a UMLAdapter interface to facilitate a new API/ caller filtering, but it won't work till all UML add-ons exploit it too.
It seems better for perhaps CDO to offer a perhaps IndestructableAdapter API so that callers can respect it.
If UML2Util.destroy is to be used anyway. The caller could wrap it in a save-CDO-adapters destroy restore-CDO-adapters.
eclipse-uml2-bot
commented
1 week ago By Eike Stepper on Jan 12, 2017 09:52
(In reply to Thorsten Schlathölter from comment #2)
[...] this is not a papyrus issue.
The stack trace suggests that Papyrus calls this UML2Util.destroy() method. Is there something in UML2, too, that calls this utility method()? Otherwise I would say it is indeed a Papyrus issue.
eclipse-uml2-bot
commented
1 week ago By Kenn Hussey on Jan 12, 2017 10:06
The PackageOperations utility class currently calls the method, but looking at the code I think this is unintentional and that the intended target method for that case was ElementOperations.destroy(EObject); I should probably fix that.
A couple of additional observations here:
The method in question is actually protected so this isn't, strictly speaking, public API
UML2Util actually knows nothing about UML; this is a collection of utilities based on EMF constructs only
There are a number of adapters, not all of which are UML-specific, that might be attached and which need to be cleaned up (to avoid potential memory leaks)
I'm curious as to exactly how the NPE is encountered here, why the protected utility is being called, and whether/how it could be made more robust so as to avoid the circumstances that lead to this exception.
eclipse-uml2-bot
commented
1 week ago By Thorsten Schlathölter on Jan 12, 2017 13:46
The method seems to be called from Papyrus only in case of repairing the zombie stereotypes by deletion. It it done by a DeleteAction class that subclasses the utility class and thus gaining access to it's protected methods.
The reason why removal of the adapter is causing the exception is that in the particular case of a CDOResource the content list is a delegating list and for the delegation to work properly the CDOLegacy adapter needs to be in a proper state. Removal of the adapter changes the state in a way that it is not possible to access the object from the content list anymore although it was never removed.
The problems would not occur if the object would be deleted before the adapters are removed. Without a doubt this is very CDO specific and since it is so essential for the CDO legacy mechanism it might really be more reasonable to protect the adapter from either being removed or changing it's state upon removal - as Ed proposed. This would solve the problem for all other utility methods that work in a similar way.
eclipse-uml2-bot
commented
1 week ago By Kenn Hussey on Jan 12, 2017 15:11
Thorsten, are you suggesting, then, that Papyrus wrap its calls to the destroy() method in a way that signals to CDO that something is being destroyed, so that it can somehow preserve state? Or that the method be overloaded in UML2 to provide an option not to clear adapters at all?
| --- | --- | | Bugzilla Link | 510302 | | Status | UNCONFIRMED | | Importance | P3 normal | | Reported | Jan 11, 2017 14:12 EDT | | Modified | Jan 12, 2017 15:11 EDT | | Version | 5.1.0 | | Reporter | Thorsten Schlathölter |
Description
When an eObject is removed using the destroy method of UML2Util all adapters of the eObject are removed by calling
eObject.eAdapters().clean();
This also removes non UML related adapters which may be needed by other frameworks such as CDO. In order to store the UML model in CDO a special CDOLegacyAdapter is attached to that object. If the adapter is detached from the object, the object is not accessible anymore by CDO. Due to this for example the the deletion of Zombie Stereotypes in Papyrus\CDO is not possible. Because the
EcoreUtil.remove(eObject);
which is performed after cleaning the adapters throws the exception shown below. \ It would be best if only UML specific adapters would be removed by the destroy method. Otherwise the UML model in general cannot safely be used together with CDO.
java.lang.NullPointerException\ at org.eclipse.emf.ecore.util.DelegatingEcoreEList.resolveProxy(DelegatingEcoreEList.java:312)\ at org.eclipse.emf.ecore.util.DelegatingEcoreEList.indexOf(DelegatingEcoreEList.java:522)\ at org.eclipse.emf.common.util.DelegatingEList.remove(DelegatingEList.java:497)\ at org.eclipse.emf.ecore.util.EcoreUtil.remove(EcoreUtil.java:3308)\ at org.eclipse.uml2.common.util.UML2Util.destroy(UML2Util.java:1208)\ at org.eclipse.papyrus.uml.modelrepair.internal.stereotypes.DeleteAction.repair(DeleteAction.java:46)\ at org.eclipse.papyrus.uml.modelrepair.internal.stereotypes.ZombieStereotypesDescriptor.repair(ZombieStereotypesDescriptor.java:228)\ at org.eclipse.papyrus.uml.modelrepair.ui.ZombieStereotypesDialog$MissingSchema.apply(ZombieStereotypesDialog.java:517)\ at org.eclipse.papyrus.uml.modelrepair.ui.ZombieStereotypesDialog$2$1.run(ZombieStereotypesDialog.java:270)\ at org.eclipse.papyrus.infra.core.utils.TransactionHelper$1.run(TransactionHelper.java:378)\ at org.eclipse.emf.transaction.impl.PrivilegedRunnable.run(PrivilegedRunnable.java:87)\ at org.eclipse.papyrus.infra.core.utils.TransactionHelper$2.run(TransactionHelper.java:394)\ at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:119)