Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 343663 - Need to improve multithread protection in EMF initialization
Summary: Need to improve multithread protection in EMF initialization
Status: RESOLVED FIXED
Alias: None
Product: WTP Java EE Tools
Classification: WebTools
Component: jst.j2ee (show other bugs)
Version: 3.2.3   Edit
Hardware: PC Windows XP
: P2 major with 1 vote (vote)
Target Milestone: 3.2.4   Edit
Assignee: Carl Anderson CLA
QA Contact: Chuck Bridgham CLA
URL:
Whiteboard: PMC_approved
Keywords:
Depends on:
Blocks: 376255 378237
  Show dependency tree
 
Reported: 2011-04-22 14:33 EDT by Carl Anderson CLA
Modified: 2012-05-02 08:13 EDT (History)
3 users (show)

See Also:
david_williams: pmc_approved+
ccc: pmc_approved? (raghunathan.srinivasan)
ccc: pmc_approved? (naci.dai)
robban.hoglund: pmc_approved+
robban.hoglund: pmc_approved+
kaloyan: pmc_approved+
ccc: pmc_approved? (cbridgha)
cbridgha: review+


Attachments
Make isInited volatile (17.25 KB, patch)
2011-04-22 15:30 EDT, Carl Anderson CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Anderson CLA 2011-04-22 14:33:48 EDT
An adopter product recently encountered the following exceptions:

 Exception in thread "Thread-7" java.lang.ExceptionInInitializerError
     at java.lang.J9VMInternals.initialize(J9VMInternals.java:222)
     at sun.misc.Unsafe.ensureClassInitialized(Native Method)
     at sun.reflect.UnsafeFieldAccessorFactory.newFieldAccessor(UnsafeFieldAccessorFactory.java:37)
     at sun.reflect.ReflectionFactory.newFieldAccessor(ReflectionFactory.java:134)
     at java.lang.reflect.Field.acquireFieldAccessor(Field.java:945)
     at java.lang.reflect.Field.getFieldAccessor(Field.java:912)
     at java.lang.reflect.Field.get(Field.java:371)
     at org.eclipse.emf.ecore.plugin.RegistryReader$EPackageDescriptor.getEPackage(RegistryReader.java:274)
     at org.eclipse.emf.ecore.impl.EPackageRegistryImpl.getEPackage(EPackageRegistryImpl.java:133)
     at org.eclipse.jst.j2ee.internal.J2EEInit$20.run(J2EEInit.java:351)
     at java.lang.Thread.run(Thread.java:736)
 Caused by: java.lang.NullPointerException
     at org.eclipse.emf.common.util.AbstractEList.getNonDuplicates(AbstractEList.java:1167)
     at org.eclipse.emf.common.util.AbstractEList.addAll(AbstractEList.java:372)
     at org.eclipse.emf.ecore.impl.EClassImpl.getEAllOperations(EClassImpl.java:924)
     at org.eclipse.emf.ecore.impl.EClassImpl.freeze(EClassImpl.java:131)
     at org.eclipse.emf.ecore.impl.EModelElementImpl.freeze(EModelElementImpl.java:100)
     at org.eclipse.emf.ecore.impl.EPackageImpl.freeze(EPackageImpl.java:203)
     at org.eclipse.jst.j2ee.jca.internal.impl.JcaPackageImpl.init(JcaPackageImpl.java:196)
     at org.eclipse.jst.j2ee.jca.JcaPackage.<clinit>(JcaPackage.java:276)
     at java.lang.J9VMInternals.initializeImpl(Native Method)
     at java.lang.J9VMInternals.initialize(J9VMInternals.java:200)
     ... 10 more

 Exception in thread "Thread-6" java.lang.ExceptionInInitializerError
     at java.lang.J9VMInternals.initialize(J9VMInternals.java:222)
     at sun.misc.Unsafe.ensureClassInitialized(Native Method)
     at sun.reflect.UnsafeFieldAccessorFactory.newFieldAccessor(UnsafeFieldAccessorFactory.java:37)
     at sun.reflect.ReflectionFactory.newFieldAccessor(ReflectionFactory.java:134)
     at java.lang.reflect.Field.acquireFieldAccessor(Field.java:945)
     at java.lang.reflect.Field.getFieldAccessor(Field.java:912)
     at java.lang.reflect.Field.get(Field.java:371)
     at org.eclipse.emf.ecore.plugin.RegistryReader$EPackageDescriptor.getEPackage(RegistryReader.java:274)
     at org.eclipse.emf.ecore.impl.EPackageRegistryImpl.getEPackage(EPackageRegistryImpl.java:133)
     at org.eclipse.jst.j2ee.internal.J2EEInit$20.run(J2EEInit.java:351)
     at java.lang.Thread.run(Thread.java:736)
 Caused by: java.lang.NullPointerException
     at org.eclipse.emf.common.util.BasicEList.contains(BasicEList.java:172)
     at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:301)
     at org.eclipse.emf.ecore.impl.ESuperAdapter.notifyChanged(ESuperAdapter.java:139)
     at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:380)
     at org.eclipse.emf.common.notify.impl.NotificationImpl.dispatch(NotificationImpl.java:1033)
     at org.eclipse.emf.common.notify.impl.NotifyingListImpl.addUnique(NotifyingListImpl.java:367)
     at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:345)
     at org.eclipse.emf.ecore.impl.EClassImpl$9.delegateAdd(EClassImpl.java:1698)
     at org.eclipse.emf.ecore.impl.EClassImpl$9.delegateAdd(EClassImpl.java:1)
     at org.eclipse.emf.common.util.DelegatingEList.addUnique(DelegatingEList.java:340)
     at org.eclipse.emf.common.notify.impl.DelegatingNotifyingListImpl.doAddUnique(DelegatingNotifyingListImpl.java:385)
     at org.eclipse.emf.common.notify.impl.DelegatingNotifyingListImpl.addUnique(DelegatingNotifyingListImpl.java:281)
     at org.eclipse.emf.common.util.AbstractEList.add(AbstractEList.java:307)
     at org.eclipse.jst.j2ee.jca.internal.impl.JcaPackageImpl.initializePackageContents(JcaPackageImpl.java:949)
     at org.eclipse.jst.j2ee.jca.internal.impl.JcaPackageImpl.init(JcaPackageImpl.java:193)
     at org.eclipse.jst.j2ee.jca.JcaPackage.<clinit>(JcaPackage.java:276)
     at java.lang.J9VMInternals.initializeImpl(Native Method)
     at java.lang.J9VMInternals.initialize(J9VMInternals.java:200)
     ... 10 more
Comment 1 Carl Anderson CLA 2011-04-22 14:39:20 EDT
The problem here is that two threads got into the body of the init() method, beyond the protection at the beginning (as per the following example):

public static JcaPackage init() {
	if (isInited) return (JcaPackage)EPackage.Registry.INSTANCE.getEPackage(JcaPackage.eNS_URI);

	// Obtain or create and register package
	JcaPackageImpl theJcaPackage =  (JcaPackageImpl)(EPackage.Registry.INSTANCE.get(eNS_URI) instanceof JcaPackageImpl ? EPackage.Registry.INSTANCE.get(eNS_URI) : new JcaPackageImpl());

	isInited = true;

The question is, how do we reduce this window any further?
Comment 2 Carl Anderson CLA 2011-04-22 15:30:23 EDT
Created attachment 193942 [details]
Make isInited volatile

One possible improvement here is to make isInited volatile.  This helps protect multithreaded/multicore accesses to this variable, since it could conceivably be cached in the various cores, and thus when one instance marks isInited as true, the other could be reading the (improperly cached) false value, and then proceed.
Comment 3 Chuck Bridgham CLA 2011-04-22 15:53:14 EDT
approved
Comment 4 Carl Anderson CLA 2011-04-22 17:16:27 EDT
Committed to HEAD for WTP 3.3 M7
Comment 5 Carl Anderson CLA 2011-04-22 18:20:20 EDT
Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.

When this exception occurs, the EMF model classes cannot be loaded/used.  (The JVM marks them as having errors, and will not try to initialize them again.)

Is there a work-around? If so, why do you believe the work-around is insufficient?

No workaround.

How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?

The Java EE JUnit bucket has been run against this fix.

Give a brief technical overview. Who has reviewed this fix?

Chuck Bridgham has reviewed this fix.  By marking these static booleans as volatile, we are insuring that the value for isInited is consistent across threads/cores, such that as soon as one thread initializes the instance, no other thread will do so.

What is the risk associated with this fix?

This is an extremely low risk fix.
Comment 6 Carl Anderson CLA 2011-04-26 11:50:05 EDT
Committed to R3_2_maintenance for WTP 3.2.4
Comment 7 Robert Höglund CLA 2012-04-05 14:35:17 EDT
Sorry, The fix does not work. I have this problem and I have analyzed you fix and see some flaws. 

In the method initalizePackageContents you have the following construction:
boolean hasLock = false;
try {
hasLock = J2EEInit.aquireInitializePackageContentsLock();
} catch (InterruptedException e) {
	J2EECorePlugin.logError(e);
}		

The problem is that you never read the hasLock variable and the code will pass the barrier and perform multiple initializations, and therefore the concurrancy problem is still a potential threat.
Comment 8 Robert Höglund CLA 2012-04-05 14:50:47 EDT
private volatile static boolean isInited = false;


And relying on the construction above do not solve the multithreading problem because of the fact that multiple threads may pass before it isInited is set to true and multiple initializations will perform concurrently.