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

Bug 376724

Summary: ExternalFoldersManager still has synchronization gaps
Product: [Eclipse Project] JDT Reporter: Markus Keller <markus.kell.r>
Component: CoreAssignee: Jay Arthanareeswaran <jarthana>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: amj87.iitr, satyam.kandula
Version: 3.8Flags: markus.kell.r: review+
Target Milestone: 3.8 M7   
Hardware: PC   
OS: Windows 7   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=527819
Whiteboard:
Attachments:
Description Flags
Proposed fix
none
Updated patch none

Description Markus Keller CLA 2012-04-13 10:40:43 EDT 
From bug 244315 comment 7:

The ExternalFoldersManager still has synchronization gaps, e.g. addFolder(..)
reads and writes pendingFolders without synchronization. The set itself should be safe, but access to the pendingFolders variables is not protected everywhere.

When I set a breakpoint on the pendingFolders field, I can see multiple threads that access this field. #getFolders() is also completely unprotected and could return different folders in different threads.


Worker-2
	at org.eclipse.jdt.internal.core.ExternalFoldersManager.addFolder(ExternalFoldersManager.java:127)
	at org.eclipse.jdt.internal.core.ExternalFoldersManager.addFolder(ExternalFoldersManager.java:112)
	at org.eclipse.jdt.internal.core.JavaProject.addToResult(JavaProject.java:2785)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2734)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2855)
	at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:1960)
	at org.eclipse.jdt.core.JavaCore.initializeAfterLoad(JavaCore.java:3917)
	at org.eclipse.jdt.internal.ui.InitializeAfterLoadJob$RealJob.run(InitializeAfterLoadJob.java:36)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)

Worker-1
	at org.eclipse.jdt.internal.core.ExternalFoldersManager.addFolder(ExternalFoldersManager.java:126)
	at org.eclipse.jdt.internal.core.ExternalFoldersManager.addFolder(ExternalFoldersManager.java:112)
	at org.eclipse.jdt.internal.core.JavaProject.addToResult(JavaProject.java:2785)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2734)
	at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2855)
	at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:1960)
	at org.eclipse.jdt.internal.core.JavaProject.isOnClasspath(JavaProject.java:2238)
	at org.eclipse.jdt.internal.ui.BuildpathIndicatorLabelDecorator.getOverlay(BuildpathIndicatorLabelDecorator.java:48)
	at org.eclipse.jdt.internal.ui.BuildpathIndicatorLabelDecorator.decorate(BuildpathIndicatorLabelDecorator.java:35)
	at org.eclipse.ui.internal.decorators.LightweightDecoratorDefinition.decorate(LightweightDecoratorDefinition.java:269)
	at org.eclipse.ui.internal.decorators.LightweightDecoratorManager$LightweightRunnable.run(LightweightDecoratorManager.java:81)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.decorate(LightweightDecoratorManager.java:365)
	at org.eclipse.ui.internal.decorators.LightweightDecoratorManager.getDecorations(LightweightDecoratorManager.java:347)
	at org.eclipse.ui.internal.decorators.DecorationScheduler$1.ensureResultCached(DecorationScheduler.java:370)
	at org.eclipse.ui.internal.decorators.DecorationScheduler$1.run(DecorationScheduler.java:330)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Comment 1 Jay Arthanareeswaran CLA 2012-04-23 01:36:59 EDT 
Interestingly some of the synchronized code was altered as part of the fix for bug 310159. I will see what is the best way to ensure thread safety.
Comment 2 Jay Arthanareeswaran CLA 2012-04-23 03:02:46 EDT 
Created attachment 214365 [details]
Proposed fix

Moved the code that instantiates pendingFolders inside a synchronized block. Also, added code to call getFolders in the singleton constructor itself to keep things simple. As a side effect of this, the external folder is always created, of course if it's not present already. 

The other alternative to tackle this, bug 310159 and bug 368152 together would be to use schedule rules, but this would create a problem with UI threads, which is mentioned here: bug 368152, comment #11.
Comment 3 Jay Arthanareeswaran CLA 2012-04-23 03:03:43 EDT 
Markus, can you please review this patch for 3.8 M7?
Comment 4 Markus Keller CLA 2012-04-24 09:52:01 EDT 
addFolder was just one example where pendingFolders is accessed without synchronization.

removePendingFolder(Object) and the guard at the beginning of createPendingFolders(IProgressMonitor) should also be synchronized to prevent data races. With that, the patch looks good to me.

However, there are still open loopholes that could be the cause for bug 244315. I don't see synchronization to ensure that cleanUp and createLinkFolder/createPendingFolders don't delete and create the same folders concurrently. But as you said, we need to be extra careful if tackle that problem, since it can lead to deadlocks via scheduling rules.
Comment 5 Jay Arthanareeswaran CLA 2012-04-24 14:13:13 EDT 
Created attachment 214487 [details]
Updated patch

Patch contains additional synchronized blocks. I can't come up with scenarios under which race conditions on these two methods would result in inconsistent state of external folders. But synchronization of assignment of pendingFolders is complete now.
Comment 6 Markus Keller CLA 2012-04-25 08:18:30 EDT 
Last patch looks good to me.
Comment 7 Jay Arthanareeswaran CLA 2012-04-27 01:15:06 EDT 
Released in master via commit http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e43c6ed09b10ae435241ad2d75fd39056b384df8
Comment 8 Satyam Kandula CLA 2012-04-30 05:14:17 EDT 
Verified for 3.8M7 using code inspection.