Community
Participate
Working Groups
I have a classpath container that normally points to a jar outside the workspace. I'm adding the option that it could alternatively use a folder of binary class files (depending on what it finds on disk). Depending on how exactly the project is initialized, we may end up in a situation where an external folder exists as a Folder-object with path .org.eclipse.jdt.core.external.folders/.link0, HOWEVER, the workspace does not know about this node and thus the NameEnvironment finds no types within this container.
Created attachment 174961 [details] test showing the problem This test fails: it shows that type p.Y cannot be resolved, although a classfile exists as p/Y.class within the folder referred to by a registered classpath container. Interestingly, the bug does not show when creating the project programmatically (as opposed to copying the directory into the workspace).
Created attachment 174964 [details] incomplete/experiemtal fix This patch reflects my current findings: In the bad situation the linked folder is initialized from: JavaProject.addToResult(..) line: 2741 JavaProject.resolveClasspath(..) line: 2688 JavaProject.resolveClasspath(..) line: 2789 JavaProject.getResolvedClasspath() line: 1915 ExternalFoldersManager.refreshReferences() line: 310 DeltaProcessor.resourceChanged(IResourceChangeEvent) line: 1891 DeltaProcessingState.resourceChanged(IResourceChangeEvent) line: 470 at this point externalFoldersManager.addFolder(resolvedPath) creates a new IFolder(bla/.link0) and adds it to ExternalFoldersManager.folders. Later, during ExternalFolderChange.updateExternalFoldersIfNecessary(..) the folder is already found in oldFolders thus preventing a call to foldersManager.createLinkFolder(..). Still later, the ClasspathDirectory representing the classpath container performs directoryList(..) which calls this.binaryFolder.findMember(..), which tries to lookup a folder in the workspace's tree. This lookup returns null, thus no class files within this folder are found. In order to back this analysis the experimental "fix" replaces externalFoldersManager.addFolder(resolvedPath) with externalFoldersManager.createLinkFolder(resolvedPath, false, null) thus eagerly registering the linked folder with the workspace. With this patch two things change: + the test passes :) + the following exception is thrown :( org.eclipse.core.internal.resources.ResourceException: The resource tree is locked for modifications. at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:115) at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:1914) at org.eclipse.core.internal.resources.Project.create(Project.java:269) at org.eclipse.jdt.internal.core.ExternalFoldersManager.createExternalFoldersProject(ExternalFoldersManager.java:237) at org.eclipse.jdt.internal.core.ExternalFoldersManager.createExternalFoldersProject(ExternalFoldersManager.java:179) at org.eclipse.jdt.internal.core.ExternalFoldersManager.createLinkFolder(ExternalFoldersManager.java:123) at org.eclipse.jdt.internal.core.JavaProject.addToResult(JavaProject.java:2741) at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2688) at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2789) at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:1915) at org.eclipse.jdt.internal.core.ExternalFoldersManager.refreshReferences(ExternalFoldersManager.java:310) at org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(DeltaProcessor.java:1891) at org.eclipse.jdt.internal.core.DeltaProcessingState.resourceChanged(DeltaProcessingState.java:470) I couldn't see anything that was broken by the exception but still it shows that there is reason to avoid modifying the workspace during addToResult. Any ideas how to fix without anything else? Do we need to record pending linked folders for deferred creation?
Created attachment 174970 [details] better fix (In reply to comment #2) > Do we need to record pending linked folders for deferred creation? In trying to answer my own question here's a fix that does this: when registering a linked folder along the "bad path" the ExternalFoldersManager stores the path in a set "pendingFolders". Later, when updateExternalFoldersIfNecessary is executed it additionally checks isPendingFolder(..) and potentially creates the linked folder at this point in time. Test now passes without exception. Does this fix make sense to you?
Created attachment 175000 [details] patch version 2 One of my test suites triggers a workspace save between tests, which revealed a related issue: ExternalFoldersManager.cleanup(..) is incomplete: it deletes linked folders without cleanup in this.folders. This patch adds such cleanup to the previous fix.
(In reply to comment #4) > Created an attachment (id=175000) [details] > patch version 2 > > One of my test suites triggers a workspace save between tests, which revealed > a related issue: ExternalFoldersManager.cleanup(..) is incomplete: > it deletes linked folders without cleanup in this.folders. > > This patch adds such cleanup to the previous fix. Patch looks good to me. I have one comment about the additional fix - Would it be better to remove the folder from 'this.folders' in the getFoldersToCleanUp itself, while collecting folders to be cleaned-up? This would avoid the additional for loop in ExternalFoldersManager#cleanUp(). Let me know if I have missed something.
Created attachment 175506 [details] test & fix version 3 (In reply to comment #5) > Patch looks good to me. I have one comment about the additional fix - Would it > be better to remove the folder from 'this.folders' in the getFoldersToCleanUp > itself, while collecting folders to be cleaned-up? This would avoid the > additional for loop in ExternalFoldersManager#cleanUp(). Let me know if I have > missed something. You're right, those loops could be folded. I didn't want to insert state change into getFolderToCleanUp(), which looked like a pure query. Also, I don't want to add deletions into the loop within getFolodersToCleanUp because that would move those deletions into the synchronized statement, which might potentially cause deadlocks - I didn't want to get into that. Thus I have changed the ArrayList from containing IFolder to storing the Map.Entry with both IPath (key) and IFolder (value). Now we can consistently cleanup both parts within a single loop. The patch extends the test to perform the workspace.save() that triggers cleanUp() as to witness this additional problem. I also updated the copyright headers :)
Thanks for the patch Stephan and sorry for the delay in getting back to this. Patch looks good. One observation about the ExternalFoldersManager.isPendingFolder() : From the signature, though this looks like a simple query, it also removes the entry from the collection. I can see that you have done this for simplicity and also for performance sake. While I don't have any problem with this at all, a little comment or a different name to the method would help. Let me know your thoughts. I will release this today.
(In reply to comment #7) > Thanks for the patch Stephan and sorry for the delay in getting back to this. > Patch looks good. > > One observation about the ExternalFoldersManager.isPendingFolder() : From the > signature, though this looks like a simple query, it also removes the entry > from the collection. I can see that you have done this for simplicity and also > for performance sake. While I don't have any problem with this at all, a little > comment or a different name to the method would help. > > Let me know your thoughts. I will release this today. Stephan, I am waiting for your comment. I know this is a small change but I think I can't make changes to your patch.
Created attachment 177533 [details] test & fix version 4 (In reply to comment #7) > Thanks for the patch Stephan and sorry for the delay in getting back to this. same here :) > One observation about the ExternalFoldersManager.isPendingFolder() : From the > signature, though this looks like a simple query, it also removes the entry > from the collection. I can see that you have done this for simplicity and also > for performance sake. While I don't have any problem with this at all, a little > comment or a different name to the method would help. I totally agree, it's the same kind of critique I raised in comment 6 :) I updated the method name and added a comment. This actually emphasizes an ananogy between the two method calls in: if (oldFolders == null || !oldFolders.remove(folderPath) || foldersManager.removePendingFolder(folderPath)) { ... thanks
Thanks for the new patch, Stephan. Released in HEAD for 3.7 M2.
Verified for 3.7M2 using build I20100909-1700
This was put into 3.6.2+, see bug 343509 for details.
