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

Bug 320450

Summary: Classpath problems because the IDependencyGraph is missing data
Product: [WebTools] WTP Java EE Tools Reporter: Jason Sholl <jsholl>
Component: jst.j2eeAssignee: Jason Sholl <jsholl>
Status: RESOLVED FIXED QA Contact: Chuck Bridgham <cbridgha>
Severity: major    
Priority: P3 CC: ccc, david_williams, raghunathan.srinivasan
Version: 3.2Flags: raghunathan.srinivasan: pmc_approved?
raghunathan.srinivasan: pmc_approved+
jsholl: pmc_approved? (naci.dai)
raghunathan.srinivasan: pmc_approved?
jsholl: pmc_approved? (neil.hauge)
jsholl: pmc_approved? (kaloyan)
david_williams: pmc_approved+
cbridgha: review+
Target Milestone: 3.2.1   
Hardware: PC   
OS: Windows Server 2003   
Whiteboard: PMC_approved
Attachments:
Description Flags
patch for 3.2.1 none

Description Jason Sholl CLA 2010-07-20 18:54:48 EDT 
As far as the user is concerned, the EAR Libraries classpath container is missing entries when users bring projects into their workspace one after another.  There underlying problem is actually the IDependencyGraph.

There are a few related problems:

1. The resource changed event that notifies when the project has be added (either through import or open) is being fired.  A bit later another resource changed event is fired signaling that the files have been added.  This second event needs to be handled from the perspective of the IDependencyGraph the same as a project being added.  This is what the change in DependencyGraphImpl's ResourceChangedListener.

2. After the IDependencyGraph determined an update was required, it was not updating properly because the underlying VirtualComponent's cache was not being cleared.  It is necessary to force a clearCache() on all components when any new project is being brought in.  Because there is already a protected clearCache() method on VirtualComponent and changing the signature would potentially break API, there is now a new public method called flushCache() on VirtualComponent.  This method is being called from two places in the DependencyGraph when it processes the updates.

3. Events were not being fired for all IDependencyGraph changes as they should have been.  This is why there is a change in getReferencingComponents() and why the event firing logic was pulled out into the notifiyListeners() method.  Also, there is no need to fire events if nothing actually changed with respect to the graph, which is why the notifiyListeners() returns if the type is 0.

4. It is possible to deadlock if a call is made to Job.yeildRule() if the workspace is treeLocked.  I have not actually hit this deadlock but was warned of it by Min.  This code has been in my local workspace for a week or so and has caused no problems, so we ought to put it in.  This is the change in waitForAllUpdates().

5. The other changes were simply to consolidate read/write of modStamp to the two private methods getModStamp() and incrementModStamp().  This was necessary during debugging, is better code style, and will help with future debugging.
Comment 1 Jason Sholl CLA 2010-07-20 18:56:11 EDT 
Created attachment 174803 [details]
patch for 3.2.1
Comment 2 Jason Sholl CLA 2010-07-20 21:05:10 EDT 
    * 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. 

The incorrect computation of the EAR Libraries classpath container is a major issue for our customers.  There may also be other side effects of an inproperly computed dependency graph.

    * Is there a work-around? If so, why do you believe the work-around is insufficient? 

Restart workbench

    * How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 

Tested by hand and tested with JUnits.

    * Give a brief technical overview. Who has reviewed this fix? 

See above.

    * What is the risk associated with this fix? 

None
Comment 3 Chuck Bridgham CLA 2010-07-21 10:42:50 EDT 
Jason I approve, but please do thorough testing on this.
Comment 4 David Williams CLA 2010-07-21 11:06:49 EDT 
Is this related to bug 319654? 

Just from glancing at patch, this looks like a pretty big changed. 

Are you saying it will be frequently encountered? And viewed as a major regression? If so, regression from 3.2.0? Or 3.1.2? (If the later, why not found until now?)
Comment 5 Jason Sholl CLA 2010-07-21 12:01:57 EDT 
This is not related to bug 319654.

From a user's perspective, this is a regression which was introduced in 3.2.1 with bug 317120.  However, technically, this problem has always existed but would rarely be encountered in 3.2 or earlier.  Bug 317120 significantly improved performance of this code.  Prior to this fix all the IResourceChanged events would have worked their way through before the processing began (because it was much slower (and deadlock prone)).

Users may encounter this problem any time they import load Java EE projects into their workspace.  The most likely scenario is if they do not load all of their related projects at the same time.  Once the projects are loaded this problem has been known to occur (though in theory could without these fixes).
Comment 6 David Williams CLA 2010-07-21 12:26:55 EDT 
Ok, thanks Jason. Since users will experience this as a regression, I'll approve. 

Test well! 

Thanks for fixing.
Comment 7 Tim deBoer CLA 2010-07-21 13:15:43 EDT 
+1. Discussed the problem and risk with Jason, and reviewed the patch. Will need lots of testing, but I approve.
Comment 8 Raghunathan Srinivasan CLA 2010-07-21 14:05:32 EDT 
Approved since the bug is a regression due to change in 3.2.1.
Comment 9 Jason Sholl CLA 2010-07-21 18:17:22 EDT 
code checked into head for 3.2.1