Summary: | Deadlock when opening RSE Preferences first time during workspace modification | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||||
Component: | RSE | Assignee: | Martin Oberhuber <mober.at+eclipse> | ||||||
Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||
Severity: | major | ||||||||
Priority: | P2 | CC: | ddykstal.eclipse | ||||||
Version: | 2.0 | Flags: | ddykstal.eclipse:
review+
|
||||||
Target Milestone: | 2.0.1 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Martin Oberhuber
2007-07-18 04:46:12 EDT
Created attachment 74035 [details]
Thread dump of the threads taking part in the deadlock
Created attachment 74036 [details]
Patch to RSEPersistenceManager fixing the issue
The fix is to simply remove the "synchronized" statement before
RSEPersistenceManager.isBusy()
RSEPersistenceManager.load()
The isBusy() method was always meant to be a method that can quickly return, it should never be blocked for long through a synchronized statement. What's more, the threading model in RSEPersistenceManager is protected through use of a Mutex anyways so there is no need for synchronized.
As a rule of thumb, synchronized blocks should ALWAYS protect very small pieces of code that are GUARANTEED to be able to complete. We should NEVER have a piece of code synchronized that calls out to other methods -- as the RSEPersistenceManager.load() method does.
This was the original reason for introducing the Mutex, so having the method synchronized in addition to that was a coding error.
Dave please review the patch for information, though I'm checking it in right away as [196919] Fix deadlock with workspace operations Yes, the patch looks good. The synchronized on load should have been removed when the mutex was added. The synchronized on isBusy was there due to a misunderstanding on how the mutex needed to be guarded. |