Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 330621

Summary: AbstractDelegatingFactory and implementations synchronized incorrectly-inefficiently
Product: [WebTools] Java Server Faces Reporter: Andrew McCulloch <Andrew.McCulloch>
Component: JSF ToolsAssignee: Carlin Rogers <carlin.rogers>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: Andrew.McCulloch, cameron.bateman, carlin.rogers, raghunathan.srinivasan, yurykats
Version: 3.2.2Flags: raghunathan.srinivasan: pmc_approved? (david_williams)
raghunathan.srinivasan: pmc_approved? (naci.dai)
deboer: pmc_approved+
raghunathan.srinivasan: pmc_approved? (neil.hauge)
raghunathan.srinivasan: pmc_approved? (kaloyan)
raghunathan.srinivasan: review+
Target Milestone: 3.2.3   
Hardware: PC   
OS: Windows XP   
Whiteboard: PMC_approved
Attachments:
Description Flags
Suggested Patch none

Description Andrew McCulloch CLA 2010-11-18 17:19:32 EST 
Build Identifier: M20100909-0800

org.eclipse.jst.jsf.context.AbstractDelegatingFactory, org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.StructuredDocumentContextResolverFactory and org.eclipse.jst.jsf.context.structureddocument.internal.impl.StructuredDocumentContextFactory all access the _delegates protected member in AbstractDelegatingFactory.  This member is a CopyOnwriteArrayList but still requires some synchronization due to a required sort after each add.  The abstract class does not synchronize around a remove call, and the implementation classes synchronize too long when they only need to synchronize around the call to retrieve an iterator.  Additionally the sync block in StructuredDocumentContextResolverFactory.delegateGetTaglibContextResolver holds the monitor while calling code getTaglibContextResolver() which can indirectly call JobManager.yeildRule() in ModelManagerImpl.  This results in deadlock prone code as another job may acquire the rule and then attempt to access the resolver factory _delegates object.  This is one of several deadlocks we have been experiencing due to calls to JobManager.yieldRule().

Reproducible: Sometimes
Comment 1 Andrew McCulloch CLA 2010-11-18 17:22:09 EST 
Here is an excerpted stack that shows the call to yield rule while inside of a synchronized block which starts in StructuredDocumentContextResolverFactory.delegateGetTaglibContextResolver

     at org.eclipse.core.internal.jobs.ThreadJob.waitForRun(ThreadJob.java:270)
     at org.eclipse.core.internal.jobs.ThreadJob.joinRun(ThreadJob.java:199)
     at org.eclipse.core.internal.jobs.JobManager.yieldRule(JobManager.java:1398)
     at org.eclipse.core.internal.jobs.InternalJob.yieldRule(InternalJob.java:600)
     at org.eclipse.core.runtime.jobs.Job.yieldRule(Job.java:709)
     at org.eclipse.wst.sse.core.internal.model.ModelManagerImpl$SharedObject.waitForLoadAttempt(ModelManagerImpl.java:139)
     at org.eclipse.wst.sse.core.internal.model.ModelManagerImpl.getExistingModel(ModelManagerImpl.java:1137)
     at org.eclipse.wst.sse.core.internal.model.ModelManagerImpl.getExistingModelForRead(ModelManagerImpl.java:1252)
     at org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.WorkspaceContextResolver.getPath(WorkspaceContextResolver.java:104)
     at org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.WorkspaceContextResolver.getProject(WorkspaceContextResolver.java:47)
     at org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.WorkspaceContextResolver.getResource(WorkspaceContextResolver.java:80)
     at oracle.eclipse.tools.common.services.util.OEPEStructuredContextResolverFactory.getTaglibContextResolver(OEPEStructuredContextResolverFactory.java:63)
     at org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.StructuredDocumentContextResolverFactory.delegateGetTaglibContextResolver(StructuredDocumentContextResolverFactory.java:263)
     at org.eclipse.jst.jsf.context.resolver.structureddocument.internal.impl.StructuredDocumentContextResolverFactory.getTaglibContextResolver(StructuredDocumentContextResolverFactory.java:222)
Comment 2 Andrew McCulloch CLA 2010-11-18 17:24:45 EST 
Created attachment 183435 [details]
Suggested Patch
Comment 3 Carlin Rogers CLA 2010-11-19 11:52:33 EST 
Thanks for the patch Andrew. Appreciate the contributions. Looks good.

Raghu, can we request PMC approval for the new protected method AbstractDelegatingFactory.getDelegatesIterator(). It's highly unlikely any adopters subclassing AbstractDelegatingFactory would have this method but since it is a change, lets check that it is not an API issue for 3.2.3. Otherwise, we can release this for 3.3
Comment 4 Raghunathan Srinivasan CLA 2010-11-19 12:25:18 EST 
* 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. 
Requesting PMC approval for an API change in the maintenance release that we believe will have minimal impact on adopters. This change is essential to fix some of the deadlocks we have seen in the adopter product. This is a stop-ship and a hot-bug request.
Adopter requesting the fix: Oracle.

* 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? 
No Junit test. Manual testing.

* Give a brief technical overview. Who has reviewed this fix? 
See description and comment 3. Reviewed by Carling Rogers.

* What is the risk associated with this fix? 
Medium
Comment 5 Carlin Rogers CLA 2011-01-13 00:47:08 EST 
The patch from Andrew McCulloch has been committed. Thanks for the contribution Andrew!