Community
Participate
Working Groups
There are several unsynchronized public methods that access and modify the nonChainingJars hash set. This is not thread safe, and can lead to infinite loops. The following stack trace snippet is an example of an infinite loop. This is based on: org.eclipse.jdt.core_3.6.0.v_A32a.jar 3XMTHREADINFO "ModalContext" J9VMThread:0x4A5D6700, j9thread_t:0x4984DF50, java/lang/Thread:0x049839A0, state:CW, prio=6 at java/util/HashMap.findNonNullKeyEntry(Bytecode PC:88(Compiled Code)) at java/util/HashMap.getEntry(Bytecode PC:33(Compiled Code)) at java/util/HashMap.containsKey(Bytecode PC:2(Compiled Code)) at java/util/HashSet.contains(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/JavaModelManager.isNonChainingJar(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:17(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:4(Compiled Code)) at org/eclipse/jdt/internal/core/JavaProject.resolveClasspath(Bytecode PC:435) at org/eclipse/jdt/internal/core/JavaProject.resolveClasspath(Bytecode PC:80) at org/eclipse/jdt/internal/core/JavaProject.getResolvedClasspath(Bytecode PC:18) at org/eclipse/jdt/internal/core/DeltaProcessingState.getRootInfos(Bytecode PC:110) at org/eclipse/jdt/internal/core/DeltaProcessingState.initializeRoots(Bytecode PC:62(Compiled Code)) at org/eclipse/jdt/internal/core/DeltaProcessor.processResourceDelta(Bytecode PC:50) at org/eclipse/jdt/internal/core/DeltaProcessor.resourceChanged(Bytecode PC:480(Compiled Code)) at org/eclipse/jdt/internal/core/DeltaProcessingState.resourceChanged(Bytecode PC:57(Compiled Code)) at org/eclipse/core/internal/events/NotificationManager$2.run(Bytecode PC:10) at org/eclipse/core/runtime/SafeRunner.run(Bytecode PC:7(Compiled Code)) at org/eclipse/core/internal/events/NotificationManager.notify(Bytecode PC:79) at org/eclipse/core/internal/events/NotificationManager.broadcastChanges(Bytecode PC:107) at org/eclipse/core/internal/resources/Workspace.broadcastPostChange(Bytecode PC:22) at org/eclipse/core/internal/resources/Workspace.endOperation(Bytecode PC:150(Compiled Code)) at org/eclipse/core/internal/resources/File.create(Bytecode PC:589(Compiled Code)) at org/eclipse/core/internal/resources/File.create(Bytecode PC:12(Compiled Code)) at org/eclipse/ui/wizards/datatransfer/ImportOperation.importFile(Bytecode PC:12(Compiled Code)) at org/eclipse/ui/wizards/datatransfer/ImportOperation.importRecursivelyFrom(Bytecode PC:36(Compiled Code)) at org/eclipse/ui/wizards/datatransfer/ImportOperation.importRecursivelyFrom(Bytecode PC:81(Compiled Code)) at org/eclipse/ui/wizards/datatransfer/ImportOperation.importRecursivelyFrom(Bytecode PC:81(Compiled Code)) at org/eclipse/ui/wizards/datatransfer/ImportOperation.importFileSystemObjects(Bytecode PC:113) at org/eclipse/ui/wizards/datatransfer/ImportOperation.execute(Bytecode PC:217) at org/eclipse/ui/actions/WorkspaceModifyOperation$1.run(Bytecode PC:5) at org/eclipse/core/internal/resources/Workspace.run(Bytecode PC:82(Compiled Code)) at org/eclipse/ui/actions/WorkspaceModifyOperation.run(Bytecode PC:27) at org/eclipse/ui/internal/wizards/datatransfer/WizardProjectsImportPage.createExistingProject(Bytecode PC:201) at org/eclipse/ui/internal/wizards/datatransfer/WizardProjectsImportPage.access$16(Bytecode PC:3) at org/eclipse/ui/internal/wizards/datatransfer/WizardProjectsImportPage$16.execute(Bytecode PC:57) at org/eclipse/ui/actions/WorkspaceModifyOperation$1.run(Bytecode PC:5) at org/eclipse/core/internal/resources/Workspace.run(Bytecode PC:82(Compiled Code)) at org/eclipse/ui/actions/WorkspaceModifyOperation.run(Bytecode PC:27) at org/eclipse/jface/operation/ModalContext$ModalContextThread.run(Bytecode PC:17) 3XMTHREADINFO "Worker-7" J9VMThread:0x48D98D00, j9thread_t:0x4984DCEC, java/lang/Thread:0x03F42A00, state:CW, prio=5 at java/util/HashMap.findNonNullKeyEntry(Bytecode PC:88(Compiled Code)) at java/util/HashMap.getEntry(Bytecode PC:33(Compiled Code)) at java/util/HashMap.containsKey(Bytecode PC:2(Compiled Code)) at java/util/HashSet.contains(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/JavaModelManager.isNonChainingJar(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:5(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:17(Compiled Code)) at org/eclipse/jdt/internal/core/ClasspathEntry.resolvedChainedLibraries(Bytecode PC:4(Compiled Code)) at org/eclipse/jdt/internal/core/JavaProject.resolveClasspath(Bytecode PC:435) at org/eclipse/jdt/internal/core/JavaProject.resolveClasspath(Bytecode PC:80) at org/eclipse/jdt/internal/core/JavaProject.getResolvedClasspath(Bytecode PC:18) at org/eclipse/jdt/internal/core/JavaProject.isOnClasspath(Bytecode PC:37) at org/eclipse/jdt/internal/ui/BuildpathIndicatorLabelDecorator.getOverlay(Bytecode PC:39(Compiled Code)) at org/eclipse/jdt/internal/ui/BuildpathIndicatorLabelDecorator.decorate(Bytecode PC:2(Compiled Code)) at org/eclipse/ui/internal/decorators/LightweightDecoratorDefinition.decorate(Bytecode PC:85(Compiled Code)) at org/eclipse/ui/internal/decorators/LightweightDecoratorManager$LightweightRunnable.run(Bytecode PC:12(Compiled Code)) at org/eclipse/core/runtime/SafeRunner.run(Bytecode PC:7(Compiled Code)) at org/eclipse/ui/internal/decorators/LightweightDecoratorManager.decorate(Bytecode PC:14(Compiled Code)) at org/eclipse/ui/internal/decorators/LightweightDecoratorManager.getDecorations(Bytecode PC:29) at org/eclipse/ui/internal/decorators/DecorationScheduler$1.ensureResultCached(Bytecode PC:87(Compiled Code)) at org/eclipse/ui/internal/decorators/DecorationScheduler$1.run(Bytecode PC:146) at org/eclipse/core/internal/jobs/Worker.run(Bytecode PC:31)
Created attachment 159734 [details] Proposed Patch I see only one place where the contents of nonChainingJars being modified - in the method JavaModelManager#addNonChainingJar, which has now been updated with a synchronized statement. There is another change I have made - the chained jars can be determined by looking at the direct references alone. No need to look further inside the references' manifest. This will reduce the operation time of JavaModelManager#saveNonChainingJarsCache. Srikanth, can review please this patch?
(In reply to comment #1) > Created an attachment (id=159734) [details] > Proposed Patch > > I see only one place where the contents of nonChainingJars being modified - in > the method JavaModelManager#addNonChainingJar, which has now been updated with > a synchronized statement. Jay, Can you please check whether precluding two concurrent writers is enough to solve the problem or whether we need to preclude concurrent readers and writers as well. (I suspect the latter to be true) > There is another change I have made - the chained > jars can be determined by looking at the direct references alone. No need to > look further inside the references' manifest. This will reduce the operation > time of JavaModelManager#saveNonChainingJarsCache. If this change is not integral to the current fix, I would recommend that this be done under a different newly raised defect. This change appears to be a performance improvement and is best carried out under a separate head and only if it is really worth it in your experiments. BTW, it appears eliminating the call to resolvedChainedLibraries and replacing it with getCalledFileNames will miss some side effects of the call to the former (Such as adding the non chaining jars to the HashSet) Not sure if this is ok - can you please check ? -- Thanks.
(In reply to comment #2) Synchronizing only the writes is not sufficient. You need to synchronize the reads as well to ensure that the reading threads see any changes that were made by the writing thread. (Synchronization is about more than just locking, it plays a big role in the Java memory model).
(In reply to comment #2) > Jay, > > Can you please check whether precluding two concurrent writers > is enough to solve the problem or whether we need to preclude > concurrent readers and writers as well. (I suspect the latter > to be true) Agreed. I will post a patch that uses synchronized Set soon. > If this change is not integral to the current fix, I would > recommend that this be done under a different newly raised > defect. This change appears to be a performance improvement > and is best carried out under a separate head and only if it > is really worth it in your experiments. No, with synchronized Set, we won't be needing this. Will take this up in a separate bug. > > BTW, it appears eliminating the call to resolvedChainedLibraries > and replacing it with getCalledFileNames will miss some side > effects of the call to the former (Such as adding the non > chaining jars to the HashSet) Not sure if this is ok - can you > please check ? -- Thanks. If you have a close look at JavaModelManager#getNonChainingJarsCache(), the modified code will be invoked only when nonChainingJars is NULL, in which case the call to addNonChainingJar is not going to be of any use.
(In reply to comment #4) > > BTW, it appears eliminating the call to resolvedChainedLibraries > > and replacing it with getCalledFileNames will miss some side > > effects of the call to the former (Such as adding the non > > chaining jars to the HashSet) Not sure if this is ok - can you > > please check ? -- Thanks. > > If you have a close look at JavaModelManager#getNonChainingJarsCache(), the > modified code will be invoked only when nonChainingJars is NULL, in which case > the call to addNonChainingJar is not going to be of any use. You are right, that makes me feel *less* queasy. I would feel a lot better about this change if there are hard numbers to justify it I guess.
Created attachment 159948 [details] Updated patch This patch uses synchronized Set for the nonChainingJars. The instance of synchronized Set that is created inside JavaModelManager#getNonChainingJarsCache is added only for consistency sake and may not really be required. This instance never gets assigned to the instance variable nonChainingJars. But that appears to be a bug to me, which I will have another look at. The nonChainingJars seems to be instantiated only once and after an invocation of JavaModdelManager#resetNonChainingJarsCache, it may never be used.
Comment on attachment 159948 [details] Updated patch This patch looks alright to me.
(In reply to comment #6) > The > nonChainingJars seems to be instantiated only once and after an invocation of > JavaModdelManager#resetNonChainingJarsCache, it may never be used. The reset should reset it to a empty synchronized set instead of resetting to null perhaps ?
(In reply to comment #8) > The reset should reset it to a empty synchronized set instead of > resetting to null perhaps ? That may not guarantee us a fully useful nonChainedJars, because the non chained jars get fully populated either during the start-up or they get added (may not e all of them) during the classpath resolution. Perhaps we should assign the newly populated Set to nonChainedJars inside JavaModelManager#getNonChainingJarsCache()? I will perhaps create a new bug after spending bit more time.
Released in HEAD for 3.6M6
Filed bug 305122 as per comment 9.
Verified for 3.6M6 using build I20100305-101 by reviewing the code
Verified.
