Community
Participate
Working Groups
Created attachment 247346 [details] .log 4.5 I20140923-0800. In order to perform some extensive testing of the fix for bug 347907, I restarted my Eclipse several times in a row without performing any other action and it turned out that in case of my Eclipse installation (SDK, EGit, Releng Tools, API Tools) all "unsaved changes" are caused by the following jobs: org.eclipse.core.internal.events.AutoBuildJob org.eclipse.core.internal.events.NotificationManager.NotifyJob
It turns out that AutoBuildJob activity is expected. I managed to capture console output of a few sessions in which after clean startup the workspace was closed with "unsaved changes". Here is what happens: 1. I asked Eclipse to close using the red 'X'. 2. During full save of workspace, /External Plug-in Libraries/.searchable was somehow modified. 3. Because of this modification, at the end of the full save, an auto-build was requested. 4. At the end of the auto-build, a snapshot was requested. 5. Since the snapshot was requested after the full save, a snapshot file was written during workspace close which resulted in "unsaved changes" during subsequent startup. Full save is a regular workspace operation so if any resource change happens during the save, another build and save will be needed. This is expected and works like that by design, but can result in "unsaved changes" if it happens just before Eclipse shuts down. The thing that is apparently wrong here is that there is some file modification during the full save. In the console output you can see the following resource delta is produced: Type: POST_CHANGE Build kind: 0 Resource: null Delta: /[*]: {} /External Plug-in Libraries[*]: {} /External Plug-in Libraries/.searchable[*]: {CONTENT} Exactly the same delta appears in all cases. The difference between clean shutdown and the one with "unsaved changes" depends on whether AutoBuildJob is either quick enough or it gets interrupted by Workspace.close in the right moment before it asks for a snapshot. We need to find the reason why /External Plug-in Libraries/.searchable gets modified during full save.
Created attachment 247387 [details] Console output 1
Created attachment 247388 [details] Console output 2
Created attachment 247389 [details] Console output 3
This bug is caused by the fix for bug 172524. I guess it surfaces now more often because our machines are now a bit faster than in 2007 when that bug was fixed and so auto-build and snapshot jobs can start sooner. Moving to PDE. A minimal workaround is to avoid modifying the file if its content is not going to change instead of modifying it each time, but the proper fix would be to avoid modifying it during FULL_SAVE.
It seems that saving .searchable could be moved to org.eclipse.pde.internal.core.SearchablePluginsManager.resetContainer() since it is always executed when fPluginIdSet is modified and it already modifies the classpath of the 'External Plug-in Libraries' project so all necessary scheduling rules and workspace locks seem to be available there.
Created attachment 247494 [details] Console output 4 Here is console output which shows that activity of NotificationManager.NotifyJob is also caused by a change in /External Plug-in Libraries/.searchable during full save on workspace.
Gerrit review: https://git.eclipse.org/r/#/c/34221/
I was reviewing this issue and I get this error while close and opening the workspace. The file .searchable was open in editor when I had closed eclipse and then reopened. 'An internal error occurred during: "Decoration Calculation"." on eclipse-SDK-N20141003-2000-win32-x86_64 without the fix applied java.lang.StackOverflowError at java.util.HashMap.areEqualKeys(HashMap.java:880) at java.util.HashMap.findNonNullKeyEntry(HashMap.java:527) at java.util.HashMap.getEntry(HashMap.java:512) at java.util.HashMap.get(HashMap.java:498) at org.eclipse.jdt.internal.core.JavaModelManager.containerGet(JavaModelManager.java:533) at org.eclipse.jdt.internal.core.JavaModelManager.getClasspathContainer(JavaModelManager.java:1925) at org.eclipse.jdt.core.JavaCore.getClasspathContainer(JavaCore.java:3247) at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2693) at org.eclipse.jdt.internal.core.JavaProject.resolveClasspath(JavaProject.java:2857) at org.eclipse.jdt.internal.core.JavaProject.getResolvedClasspath(JavaProject.java:1962) at org.eclipse.jdt.internal.core.ProjectReferenceChange.updateProjectReferencesIfNecessary(ProjectReferenceChange.java:47) at org.eclipse.jdt.internal.core.ChangeClasspathOperation.classpathChanged(ChangeClasspathOperation.java:59) at org.eclipse.jdt.internal.core.SetContainerOperation.executeOperation(SetContainerOperation.java:110) at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:729) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2312) at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:794) at org.eclipse.jdt.internal.core.JavaModelManager$9.run(JavaModelManager.java:2832) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2312) at org.eclipse.jdt.internal.core.JavaModelManager.initializeAllContainers(JavaModelManager.java:2848)
Created attachment 247863 [details] This error is repeatable all the time.
(In reply to Vikas Chandra from comment #9) > I was reviewing this issue and I get this error while close and opening the > workspace. The file .searchable was open in editor when I had closed eclipse > and then reopened. 'An internal error occurred during: "Decoration > Calculation"." on eclipse-SDK-N20141003-2000-win32-x86_64 without the fix > applied I have never seen it before. It looks like a different problem than this one. Please open a new bug and attach full stack trace there.
1) private static void saveStates(Set<String> newSet) { can be changed to private void saveStates() . The call to it can be changed to saveStates(); 2) I am not sure if removing ISaveParticipant is the best way here. That can probably be called with null savingcontext from resetContainer. So saving(ISaveContext context) would be same as saveStates(null). or we can comment/remove the contents of saving(ISaveContext context) and keep saveStates(). My 1st instinct for fixing this one was to put private static boolean searchablePluginsChanged = false; and then putting if (searchablePluginsChanged == false) return; at the start of saving(ISaveContext context)
...contd The only thing that will happen is that the save will happen at the workspace exit and not for individual add and remove of plugin from java search ( as happening with current patch). This approach will have minimal change. But logically it makes more sense when the save happens in resetContainer.
(In reply to Vikas Chandra from comment #12) > 1) private static void saveStates(Set<String> newSet) { can be changed to > private void saveStates() . The call to it can be changed to saveStates(); I thought it's better to keep both load and save methods static but I can change that if you prefer save to be non-static. I will push updated patch shortly. > 2) I am not sure if removing ISaveParticipant is the best way here. That > can probably be called with null savingcontext from resetContainer. So > saving(ISaveContext context) would be same as saveStates(null). > > or we can comment/remove the contents of saving(ISaveContext context) and > keep saveStates(). Please note that ISaveParticipant.saving(ISaveContext) has the following sentence in its javadoc: "This method is called by the platform; it is not intended to be called directly by clients." So in general PDE shouldn't be calling it directly. Also per convention we allow null arguments only if null is allowed and directly mentioned in the javadoc. The resources bundle never calls ISaveParticipant methods with null so implementing ISaveParticipant in this case looks like a misuse. But I can change the patch if you strongly prefer to keep ISaveParticipant there as long as ".searchable" will not get modified during workspace save.
>> But I can change the patch if you strongly prefer to keep ISaveParticipant No we are looking for the best way ahead. I am more or less in agreement with the solution as mentioned before. Also I agree putting null could be a misuse. >Please note that ISaveParticipant.saving(ISaveContext) has the following >sentence in its javadoc: >"This method is called by the platform; it is not intended to be called >directly by clients." But clients may implement this interface * <p> * Clients may implement this interface. * </p> The question is - whether we can remove an interface? What if a client used it directly in his/her code? The best solution "could" be just to retain the interface ( dummy) with no contents and have saveStates() public void doneSaving(ISaveContext context) { // nothing is required here } public void prepareToSave(ISaveContext context) { // no need for preparation } public void rollback(ISaveContext context) { // do nothing. not the end of the world. } public void saving(ISaveContext context) throws CoreException { } Ideally we want it removed but if and only if we are sure about it. Any other suggestions?
(In reply to Vikas Chandra from comment #15) > The question is - whether we can remove an interface? What if a client used > it directly in his/her code? ... > Ideally we want it removed but if and only if we are sure about it. SearchablePluginsManager is an internal class (inside the org.eclipse.pde.internal.core package) so I would say we can do with it anything we want.
Oh yes .. The package is org.eclipse.pde.INTERNAL.core; Ok, so I think just the change 1) would suffice. a) no need of static method b) no need of argument in saveStates The junit runs just fine ! That should be good to go.
I was trying to commit patch 3 but because of hudson build issues, I am not getting that option. Will wait for that issue to get resolved.
(In reply to Vikas Chandra from comment #18) > I was trying to commit patch 3 but because of hudson build issues, I am not > getting that option. Will wait for that issue to get resolved. You have to remove the hudson reviewer. I have done that, so you should be able to submit now.
Commited patch 3 of https://git.eclipse.org/r/#/c/34221/ via http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=856a50c44501a71c9948f5fb4ed72fff2bcff294 I tried to recreate problem in #comment9 and #comment10 today but in vain.
Fixed via commit in previous comment.
Szymon, can you please verify that this works in any eclipse 4.5 build after 16th october?
(In reply to Vikas Chandra from comment #22) > Szymon, can you please verify that this works in any eclipse 4.5 build > after 16th october? Sure. Verified in I20141021-0800 that .searchable is no longer modified during FULL_SAVE. Thanks for help, Vikas!
