Community
Participate
Working Groups
This has been observed in a product adopting Eclipse, where managing working sets is automated. High frequency of background changes to working sets exposes the risk of concurrent access to the WorkingSetManager.saveState. It raises an error dialog, which is irritating in the long run. Older Eclipse releases silently logged the exception, but this is not the proper solution. AbstractWorkingSetManager#saveState(File) should be synchronized.
Created attachment 179958 [details] Patch
Created attachment 179959 [details] Better patch
The patch has been tested in production environment and it solves the problem. Because of intrusiveness of the dialog, the patch needs to be released into Eclipse 3.6.2. Hitesh, could you help me with the process? Who should approve this?
(In reply to comment #3) > The patch has been tested in production environment and it solves the problem. > > Because of intrusiveness of the dialog, the patch needs to be released into > Eclipse 3.6.2. > > Hitesh, could you help me with the process? Who should approve this? My apologies, looks like my mail client has been missing bugzilla updates. I'll try to get back to you on this asap.
(In reply to comment #0) > This has been observed in a product adopting Eclipse, where managing working > sets is automated. High frequency of background changes to working sets exposes > the risk of concurrent access to the WorkingSetManager.saveState. The working-set manager was not designed with thread-safety in mind, see Bug 272528 . Also, the code looks prone to deadlocks if locks are used.
Those looks in the code are innocent - no 3rd party is called.
I assume the patch has been released to CVS HEAD already, right ? The lock could have been obtained on the state file itself rather than the manager ( wonder why not ?). Anyway, I am not aware of the details of the bug and the reasons for NPE , and hence I cannot comment further ... Paul, feel free to update the QA contact as appropriate; for time being I am setting you as the QA contact for this bug.
The file is deleted if any error happens - it may be null. Therefore it is necessary to synchronize also conditions like stateFile == null or stateFile.exists. stateFile may be also null. The exception happens when the workingSet is frequently altered, possibly from different threads. If two threads write to the same file, anb IOException is thrown. The state file is deleted (another exception may happen if other thread is writing) and then recreated by other thread.
Hitesh, please vette the change and make sure it won't cause deadlock, and work with Christopher to provide a test that can reproduce the problem. It's going into a maintenance release, and that means it 1) must be ultra safe and 2) can only go in with a committer review *and* component lead approval. PW
Thanks Paul. Bug 293882 Comments 6 through 9 try to highlight the potential problem. We still do not get thread safety with the patch. For example, the sortedset 'workingSets' can be easily subject to concurrent access. Or notice how restore will call property listeners. And ,again, changing any workingset will make a call to save method, which highlights the potential for problems. workingSetChanged(IWorkingSet,String,Object)->saveState() I have seen client code which tries to catch lock on workingset manager itself...
Hitesh, I am not trying to achieve full thread safety, I just want to solve the exception issue. the restoreState method is called only once in WorkbenchPlugin.getWorkingSetManager. It cannot cause any problems unless synchronized was locked on working set manager. But this is impossible, because getWorkingSetManager must be called, so manager will be initialized before any lock can be put on it. SaveState and restoreState don't call any 3rd party code. They are guaranteed to finish (once invoked), therefore no deadlock can happen. The only problem I can see is the fact that a client could possibly lock the manager (as you mentioned) and hold the lock forever. Other threads will not be able to modify any working set. I wonder if this is a proper scenario, though. Do you think that introducing custom Object, on which the lock will be held during save will be better?
(In reply to comment #11) > Hitesh, > > I am not trying to achieve full thread safety, I just want to solve the > exception issue. The point is that you may still continue to get exceptions (just like in the case of that AIOBE bug). I am not sure what exactly causes NPE here - it could well be due to concurrent access to the set (as was the case for AIOBE bug). > > the restoreState method is called only once in > WorkbenchPlugin.getWorkingSetManager. This is not necessarily the case. For example, JDT, I guess, uses its own instance of WorkingsetManager, and can choose to call restore several times during a run...there are several possibilities. >It cannot cause any problems unless > synchronized was locked on working set manager. But this is impossible, because > getWorkingSetManager must be called, so manager will be initialized before any > lock can be put on it. > > SaveState and restoreState don't call any 3rd party code. They are guaranteed > to finish (once invoked), therefore no deadlock can happen. > > The only problem I can see is the fact that a client could possibly lock the > manager (as you mentioned) and hold the lock forever. Other threads will not be > able to modify any working set. I wonder if this is a proper scenario, though. > Nothing which prevents them either :) > Do you think that introducing custom Object, on which the lock will be held > during save will be better? If mutual-exclusion just between the two methods is desired then one can think of using something that is exclusively used by the two (the two methods are locked-out against each other for sure). But I notice that several other variables are used, and they still remain unprotected. Another problem with just making the two synchronized is that it would send out the wrong message - at the moment workingsetManager is not thread-safe. If it is desired one can do it the way other clients have done it( or if planned, go the Collections. synchronizedCollection(c) way, which is a more general issue).
Created attachment 181653 [details] Safer Fix Hitesh, You are right that either full or none thread safety is delivered. I have prepared much simpler patch - since the exception is just annyoing and has almost no consequences, we can just log it without opening the error dialog. I will continue fixing the exception on client side - let's just make it less annyoing.
I will of course revert previous patch.
(In reply to comment #13) > Created an attachment (id=181653) [details] > Safer Fix (In reply to comment #14) > I will of course revert previous patch. > I will continue fixing the exception on client side - let's just make it less > annyoing. I personally do not mind not showing the error every time. Multithreaded access is not supported in the first place and should be fairly uncommon. If there exist a bug related to this we still have the logs.
Paul, can this go into 3.6.2?
We just need Boris' approval. PW
(In reply to comment #13) > You are right that either full or none thread safety is delivered. > I have prepared much simpler patch - since the exception is just annyoing and > has almost no consequences, we can just log it without opening the error > dialog. > > I will continue fixing the exception on client side - let's just make it less > annyoing. +1 from me for 3.6.2: adding synchronize to a method wouldn't have been the right answer for a maintenance release, but logging instead of showing is. Unless we believe the system becomes unstable when the error occurs, logging is better than showing because the end user cannot do anything about it anyway. Please make sure you enter a new bug for tracking the root of the problem.
Hitesh, could you release the patch into 3.6.2? I could do it myself, but I do not have access to releng :-(
(In reply to comment #19) > Hitesh, could you release the patch into 3.6.2? I could do it myself, but I do > not have access to releng :-( Chris, Oleg will be doing a 3.6.2 release next Tuesday so you could release the fix into R3_6_maintenance and it will be picked up. PW
Ping.
Patch is released into 3.6 maintenance stream. ui.map file is updated.
