Community
Participate
Working Groups
I found this problem while attempting to fix what appeared to a problem with the Web App Libraries classpath container defined in WTP. This container automatically automatically adds all jars residing in the project's WEB-INF/lib folder. There is also listening code which reacts to when jars are added or removed from this folder so the container is reset any time a user changes these jars. The problem I was digging into was if one of the existing jars was replaced (exact same name, but different contents), the container did not seem to update. Initially, I enhanced the listening code to catch the jar replace scenario and ensure the container was replaced (i.e calling JavaCore.setClasspathContainer with a new instance of the container), this did not fix the classpath problems. I observed the following when replacing jars handled by this container regardless of whether the container updated (i.e. I observed exactly the same behavior with and without my fixes). My testcase was to have two jars named "stuff.jar" one contained the class test.A, the other contained test.B. My Web App contained two classes, called NeedsA and NeedsB which each extended their respective classes in stuff.jar (so only one of these can actually compile at a time). Then, I simply dropped one of the stuff.jars in the WEB-INF/lib folder, and then replaced it with the other. First, the underlying JDT compile model seems to be correct. E.g. the markers view shows the correct errors or lack thereof and the package explorer's class label decorators also show those errors or lack thereof. Second, the editor's markers are stale and stay that way. Third, the contents of the classpath container's jar in the PackageExplorer when the container is expanded and the jar is expanded is also stale and stays that way. Unfortunately, there does not seem to be any workaround for the user, aside from removing and readding the container or closing and reopening the project to bring all aspects of the JDT model back in sync. JDT should treat jars added by containers exactly the same as jars added directly to the classpath (I performed the same sort of test of replacing stuff.jar when it was added directly to the classpath, and that always seemed to work). This is a major problem because an adopter built on top of WTP using this container has some tooling which generates/updates jars that reside in the WEB-INF/lib folder.
Created attachment 161479 [details] example I used to reproduce
What build are you using ?
not sure eclipse build, but it contains org.eclipse.jdt.core_3.6.0.v_A35.jar which is newer than 36M5.
Able to reproduce with build 20100204-0846 (eclipse-jee-helios-M5-win32). However, when I use a plain eclipse (build I20100309-0809) and a java project with a user library (including stuff.jar) to create a similar scenario, I am not seeing the bug. That is, any change to the JAR is considered when I refresh. However, in the previous case, even changing the name of the WEB-INF\lib\stuff.jar doesn't impact the project. I am investigating.
(In reply to comment #4) > That is, any change to the JAR is considered when I > refresh. However, in the previous case, even changing the name of the > WEB-INF\lib\stuff.jar doesn't impact the project. I am investigating. Please forgive that statement. I realize how silly it might have sounded. Anyhow, I tried this once again and can't reproduce the bug! Probably I was copying the JAR to a wrong location in the previous try. When I refresh the project from package explorer, the correct JAR seem to be picked up and problem view is updated appropriately. It's the same build (20100204-0846) and result is same with both the following approaches: 1. Creating a new Dynamic Web project and then over-writing with the provided contents. 2. Importing the given as an Existing Java project and then adding the Web App Libaries to that project. Anything missing here?
No I think you have the basic steps. The only difference is the classpath container we're seeing this on is a customer classpath container defined in WTP, so I'm not sure if you can replicate with a simple user library.
Jason, please make sure you provide a good test case to Jay in order to reproduce this issue and get it fixed. Without appropriate information this is unlikely to be fixed for 3.6.
This example is adequate to reproduce the problem, which is still present on the M6 drivers. The simplest way to reproduce this would be for you to get the M6 WTP driver here: http://build.eclipse.org/webtools/committers/ Here's the full link: http://build.eclipse.org/webtools/committers/wtp-R3.2.0-S/20100315195309/S-3.2.0M6-20100315195309/ You will need to unzip the following zips into your target. See screenshot. Once you do that, simply follow the steps.
Created attachment 164062 [details] how to setup your target
(In reply to comment #9) > Created an attachment (id=164062) [details] > how to setup your target Thanks Jason. I am able to reproduce now. On further investigation, the classpath resolution is alright. It's strange that the expanded JAR in the package explorer still shows the old content. Moving to JDT/UI for further investigation.
Chatted with Jay: he will check whether the Package Explorer receives the correct Java element delta.
(In reply to comment #0) > First, the underlying JDT compile model seems to be correct. E.g. the markers > view shows the correct errors or lack thereof and the package explorer's class > label decorators also show those errors or lack thereof. > > Second, the editor's markers are stale and stay that way. > > Third, the contents of the classpath container's jar in the PackageExplorer > when the container is expanded and the jar is expanded is also stale and stays > that way. I see the problem only with project explorer and package explorer seem to be working fine. Can you please try this and confirm?
Have some update after trying the same use-case from both Package Explorer and Project explorer. This is the delta being received by PackageExplorerContentProvider#processDelta() 1) When refreshed from Project explorer: Java Model[*]: {CHILDREN} Web[*]: {CONTENT} ResourceDelta(/Web/WebContent)[*] 2) Package explorer: Java Model[*]: {CHILDREN} Web[*]: {CHILDREN} C:\JDT\runtime-New_configuration_orig\Web\WebContent\WEB-INF\lib\stuff.jar[*]: {CONTENT | ARCHIVE CONTENT CHANGED} Interestingly in both cases, this method is invoked from different parts of the JDT/Core code. The former comes from workspaces broadcastPostChanges() while the latter gets invoked through org.eclipse.jdt.ui.actions.RefreshAction (via DeltaProcessor#checkExternalArchiveChanges). More interestingly, no signs of RefreshAction at all in the case of ProjectExplorer. Continuing to investigate further.
Update: This happens to all containers with external path. The fix might be either 1) to find out why RefreshAction is not invoked from project explorer (if it's not the intended behavior already) and fix that code or 2) In DeltaProcessor (during POST_CHANGE event), check if a jar with external path has changed and call DeltaProcessor#checkExternalArchiveChanges. I haven't thought much about this option yet and I am not very sure about the complexity of this fix. Dani, what's your opinion about the first?
(In reply to comment #14) > Update: This happens to all containers with external path. The fix might be > either > 1) to find out why RefreshAction is not invoked from project explorer (if it's > not the intended behavior already) and fix that code or > 2) In DeltaProcessor (during POST_CHANGE event), check if a jar with external > path has changed and call DeltaProcessor#checkExternalArchiveChanges. I haven't > thought much about this option yet and I am not very sure about the complexity > of this fix. > > Dani, what's your opinion about the first? Well, it appears that there is no way for the DeltaProcessor to find out that the particular JAR is a different one - even if we compare the timestamp, it still doesn't address the hypothetical scenario where the JARs have the same name and timestamp but different content. I just confirmed that when the timestamp and name are same, the refresh doesn't happen from Project explorer not just for the external JAR but also for the in-project ones. I think the JDT/UI might need a RefreshAction here.
> I think the JDT/UI might need a RefreshAction here. I'm not sure what you mean here - can you explain in more detail. The refresh action is owned by the Project Explorer because it acts on many different kinds of selected elements. We would probably need more support from Project Explorer (see bug 298676). Q: I assume it does also not work when doing refresh from Navigator view, right?
(In reply to comment #16) > > I think the JDT/UI might need a RefreshAction here. > I'm not sure what you mean here - can you explain in more detail. The refresh > action is owned by the Project Explorer because it acts on many different kinds > of selected elements. We would probably need more support from Project Explorer > (see bug 298676). I meant the class org.eclipse.jdt.ui.actions.RefreshAction and in particular the RefreshAction#run(IStructuredSelection). In the case of package explorer, this gets triggered, which takes care external libraries being refreshed (comment #13). I wasn't aware that RefreshAction is not involved in the case of Project Explorer. > > Q: I assume it does also not work when doing refresh from Navigator view, > right? Unfortunately I can't expand the JAR file from the navigator and see the contents.
>Unfortunately I can't expand the JAR file from the navigator and see the >contents. Nope, but you can trigger the refresh/F5 on the Navigator and then verify in the Package Explorer ;-)
(In reply to comment #16) > Q: I assume it does also not work when doing refresh from Navigator view, > right? No, it doesn't work.
I personally think that the current design where the client has to create a special refresh action (like we currently do for the Package Explorer) is doomed to fail - as we see here. I think JDT Core should handle this case by hosting similar code as we currently have in org.eclipse.jdt.ui.actions.RefreshAction.refreshJavaElements(IStructuredSelection, IProgressMonitor). This would 1. fix this bug 2. reduce code overhead in JDT UI as we could use the workspace refresh action without additional bloat around it
Jay, this looks like a dup of bug 255174.
I am working on a fix that takes care of refreshing external archives in case of a PRE_REFRESH event.
(In reply to comment #22) > I am working on a fix that takes care of refreshing external archives in case > of a PRE_REFRESH event. The fix may not straight forward as I thought. While we are in the PRE_REFRESH notification cycle, it's not possible to modify the workspace/project as the workspace tree has been locked. Will investigate to see if there is a way out.
Created attachment 180488 [details] Proposed patch This is the approach I find most appropriate here. Patch needs some clean-up and tests are currently being run. With this patch, we may no longer need some of the code we have in org.eclipse.jdt.ui.actions.RefreshAction.refreshJavaElements(). Will update after running some tests.
Created attachment 180576 [details] Updated patch All tests pass. Few more tests modified in JavaElementDeltaTests - all explicit calls to refreshExternalArchives() have been replaced with refresh(). Markus, as I mentioned, with this patch we might no longer need the code in RefreshAction. We can use this as the generic code for views such as Package Explorer, Projects Explorer, Navigator etc. But I will wait for your confirmation after testing this patch. TIA.
(In reply to comment #25) The Job name must be externalized. I don't see why the Job implementation uses completely different code than the original loop. Please extract this into a static helper method (keeping the old way of printing the debug statement before touching a project). The progress monitor also looks like it could need some love (should pass sub-monitors to IResource#touch(..)). Doesn't this fix make calls to IJavaModel#refreshExternalArchives(IJavaElement[], IProgressMonitor) redundant? I think that method should be deprecated when this fix is released. Or at least, it should tell that clients don't have to call it any more since the model now refreshes automatically on resource changes. I saw that you've removed calls to JavaModel#refreshExternalArchives(..) from the tests. Is that really necessary? Couldn't you keep them and add new tests for the new functionality? What's the connection between this fix and ExternalFoldersManager#refreshReferences(IProject[], IProgressMonitor)? Couldn't external archives be handled in that infrastructure as well? Why is DeltaProcessor#checkExternalArchiveChanges(IJavaElement[], boolean, IProgressMonitor) public? I'll test the patch when these problems have been discussed/solved in JDT/Core. Jay, do you have steps to reproduce in a standalone SDK, or do I have to install WTP to see the problem?
(In reply to comment #26) > Jay, do you have steps to reproduce in a standalone SDK, or do I have to > install WTP to see the problem? You could try the steps given in bug 325414, comment 11. You basically need to have a project which depends on an external JAR, whose changes don't get picked up when refresh is invoked either from the navigator or project explorer.
Created attachment 180858 [details] Updated patch Updated patch incorporating some of Markus's suggestions.
(In reply to comment #26) > (In reply to comment #25) > > The Job name must be externalized. I don't see why the Job implementation uses > completely different code than the original loop. Please extract this into a > static helper method (keeping the old way of printing the debug statement > before touching a project). The progress monitor also looks like it could need > some love (should pass sub-monitors to IResource#touch(..)). I am assuming you meant the IProgressMonitor.subTask(). Updated the patch. > Doesn't this fix make calls to > IJavaModel#refreshExternalArchives(IJavaElement[], IProgressMonitor) redundant? > I think that method should be deprecated when this fix is released. Or at > least, it should tell that clients don't have to call it any more since the > model now refreshes automatically on resource changes. Agreed, we can deprecate this. We should also remove the code in JDT/UI that uses this code. > I saw that you've > removed calls to JavaModel#refreshExternalArchives(..) from the tests. Is that > really necessary? Couldn't you keep them and add new tests for the new > functionality? Fixed the tests and added new test. > What's the connection between this fix and > ExternalFoldersManager#refreshReferences(IProject[], IProgressMonitor)? > Couldn't external archives be handled in that infrastructure as well? Not much. The ExternalFolderManager offers specific functionality w.r.t the external folders and not archives. Besides the DeltaProcessor already has most of the required code for our requirement. So, I guess we can just keep it this way to minimize the changes. > Why is DeltaProcessor#checkExternalArchiveChanges(IJavaElement[], boolean, > IProgressMonitor) public? It need not be public. Made it private.
Sorry, I didn't get to review this in time, and we shouldn't risk something for M3. I'll review it early in M4. We also need to be sure that there's no performance impact, e.g. when multiple projects are refreshed (by the user or by an auto-refresh job).
*** Bug 325414 has been marked as a duplicate of this bug. ***
(In reply to comment #30) > Sorry, I didn't get to review this in time, and we shouldn't risk something for > M3. I'll review it early in M4. I agree not to rush it in for M3. This is too sensitive.
> > The progress monitor also looks like it could need > > some love (should pass sub-monitors to IResource#touch(..)). > > I am assuming you meant the IProgressMonitor.subTask(). Updated the patch. No, I meant you should not reuse the same IProgressMonitor for multiple clients. The policy is that a method that receives a monitor can expect to get a fresh monitor, report progress in it, and call done() in the end. But you're passing the same monitor to multiple clients. The right pattern would be to pass a new SubProgressMonitor(monitor, ticks). > > IJavaModel#refreshExternalArchives(IJavaElement[], IProgressMonitor) We still have legitimate clients of this method, so I wouldn't deprecate it any more, but add a comment that tells that since 3.7, a project refresh automatically triggers a refresh of external archives, so this method doesn't need to be called any more after a project refresh. Unfortunately, we can't get rid of jdt.ui's RefreshAction, because it's an API class, and because we still need it to refresh PackageFragmentRootContainer and external IPackageFragmentRoots (which don't have an IResource, so they are not refreshed by the platform action). > > What's the connection between this fix and > > ExternalFoldersManager#refreshReferences(IProject[], IProgressMonitor)? > > Couldn't external archives be handled in that infrastructure as well? > > Not much. The ExternalFolderManager offers specific functionality w.r.t the > external folders and not archives. Besides the DeltaProcessor already has most > of the required code for our requirement. So, I guess we can just keep it this > way to minimize the changes. An important property of the ExternalFolderManager is that it refreshes all external folders together in one batch. We had quite a few performance problems there, because we had too many refreshes. The patch looks like it introduces the same problems for external archives: When I set up multiple projects that refer to the same external JAR and then refresh them all (in the Navigator), then I see separate PRE_REFRESH events for each project. But I guess you're lucky, because DeltaProcessor#createExternalArchiveDelta(..) caches the getExternalLibTimeStamps in the deltaProcessor's state, and only that avoids multiple refresh jobs that touch the projects over and over again. - Implementation note: Either remove the "if (projectsToTouch.length> 0) {" condition or move the touchJob creation into the body. - typo in Messages.updating_external_arhives_jobName Go from me with the mentioned changes and if Olivier agrees.
Created attachment 183740 [details] Updated patch (In reply to comment #33) > No, I meant you should not reuse the same IProgressMonitor for multiple > clients. Done. > We still have legitimate clients of this method, so I wouldn't deprecate it > any more, but add a comment that tells that since 3.7, a project... Done. > The patch looks like it introduces the same problems for external archives: > When I set up multiple projects that refer to the same external JAR and then > refresh them all (in the Navigator), Yes, and as has been observed in bug 302295 and bug 313153, we can't do much about multiple PRE_REFRESH events. And as you mentioned, since subsequent calls to DeltaProcessor#createExternalArchiveDelta return false, we don't have to do what was done for bug 313153. > - Implementation note: Either remove the "if (projectsToTouch.length> 0) {" > condition or move the touchJob creation into the body. > > - typo in Messages.updating_external_arhives_jobName Done. As an additional change, I have removed the message property and kept only the job name property. I feel a message doesn't have much value here. Olivier, do you have any comments?
Looks good to me.
Released in HEAD for 3.7 M4.
Released updates in org.eclipse.jdt.ui.actions.RefreshAction to HEAD.
Thanks for fixing this.
*** Bug 255174 has been marked as a duplicate of this bug. ***
Verified for 3.7M4 using build I20101205-2000
*** Bug 346928 has been marked as a duplicate of this bug. ***
*** Bug 356417 has been marked as a duplicate of this bug. ***
Created attachment 204444 [details] Patch for R3_4_maintenance Back-porting this fix to R3_4_maintenance branch.
(In reply to comment #43) > Created attachment 204444 [details] > Patch for R3_4_maintenance > > Back-porting this fix to R3_4_maintenance branch. Released in R3_4_maintenance
Created attachment 204488 [details] Patch for R3_5_maintenance CVS patch for R3_5_maintenance branch.
Created attachment 204531 [details] Patch for R3_6_maintenance This is a Git patch for the R3_6_maintenance branch.
Released the fix in R3_5_maintenance and R3_6_maintenance
