Bug 320618

Comment 1 Stephan Herrmann 2010-07-22 08:37:48 EDT

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).

Comment 2 Stephan Herrmann 2010-07-22 08:56:23 EDT

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?

Comment 3 Stephan Herrmann 2010-07-22 09:20:51 EDT

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?

Comment 4 Stephan Herrmann 2010-07-22 13:05:17 EDT

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.

Comment 5 Jay Arthanareeswaran 2010-07-26 06:47:09 EDT

(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.

Comment 6 Stephan Herrmann 2010-07-29 11:16:06 EDT

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 :)

Comment 7 Jay Arthanareeswaran 2010-08-23 03:57:38 EDT

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.

Comment 8 Jay Arthanareeswaran 2010-08-26 04:40:09 EDT

(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.

Comment 9 Stephan Herrmann 2010-08-26 11:16:52 EDT

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

Comment 10 Jay Arthanareeswaran 2010-08-30 07:28:05 EDT

Thanks for the new patch, Stephan.

Released in HEAD for 3.7 M2.

Comment 11 Satyam Kandula 2010-09-15 02:08:14 EDT

Verified for 3.7M2 using build I20100909-1700

Comment 12 Dani Megert 2011-07-27 05:02:11 EDT

This was put into 3.6.2+, see bug 343509 for details.