Bug 196919 - Deadlock when opening RSE Preferences first time during workspace modification
Summary: Deadlock when opening RSE Preferences first time during workspace modification
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows XP
: P2 major (vote)
Target Milestone: 2.0.1   Edit
Assignee: Martin Oberhuber CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-18 04:46 EDT by Martin Oberhuber CLA
Modified: 2007-07-19 12:40 EDT (History)
1 user (show)

See Also:
ddykstal.eclipse: review+


Attachments
Thread dump of the threads taking part in the deadlock (11.03 KB, text/plain)
2007-07-18 04:47 EDT, Martin Oberhuber CLA
no flags Details
Patch to RSEPersistenceManager fixing the issue (1.73 KB, patch)
2007-07-18 04:52 EDT, Martin Oberhuber CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2007-07-18 04:46:12 EDT
All of Eclipse got unresponsive in Window > Preferences > Remote Systems.

Here is what I did exactly:
* Launch Eclipse on an old RSE workspace, where Profiles are still in the
  RemoteSystemsConnections workspace project.
  Ensure that RSE perspective is NOT shown, quit Eclipse
* Restart Eclipse --> No RSE plugin is loaded.
* Start something that changes workspace resources. In my case, I launched
  a Java application in debug mode.
* With that operation still ongoing, open RSE Preferences.

Here is my analysis of the situation (full thread dump will be attached):
(1) Since RSE was never active before, opening the Preferences leads to running
    the InitRSEJob, which leads to restoring Profiles. Restoring Profiles has
    a SchedulingRule on the Workspace, so it cannot proceed until the Workspace
    is freed from the ongoing Debug Launch.

(2) Part of the Debug Launch modifies the workspace, so the RSETempFileListener
    is called. The TempFileListener calls RSEPersistenceManager.isBusy() but
    cannot complete at that time since isBusy() is synchronized and the
    Object is locked because RSEPersistenceManager.load() is also synchronized.

    --> The TempFileListener never returns, so the WorksapceModifyOperation
        never completes, Launch cannot complete and the Profile Loading is
        stuck -- we have a classic deadlock.

(3) From the UI thread (main), Preferences page wants to load a FieldEditor,
    which initially creates a SubSystemConfiguration. This leads to creating
    Filter Pools, which needs to load Profiles but cannot do so because the
    Persistence Manager cannot complete its loading.
    I think this UI block could be fixed through bug #181939 comment 12, 
    when the field editor only needs a SubSystemConfigurationProxy rather
    than the ISubSystemConfiguration. But this still does not solve the 
    deadlock in the Workspace.

The good news is that the deadlock can only happen when RSE Profiles are in 
the Workspace, which is not the default with TM 2.0 so this is unlikely to
occur unless somebody migrated on old RSE 1.x workspace.

-----------Enter bugs above this line-----------
TM 2.0
installation : eclipse-SDK-3.3 (I20070625-1500), cdt-4.0.0, emf-2.3.0
RSE install  : workspace HEAD
java.runtime : Sun 1.6.0_01-b06
os.name:     : Windows XP 5.1, Service Pack 1
------------------------------------------------
systemtype   : Windows-local, Dstore-win, Dstore-linux
targetos     : Red Hat Enterprise Linux WS release 4 (Nahant Update 3)
targetuname  : Linux parser 2.6.9-34.EL #1 i686 athlon i386 GNU/Linux
targetvm     : Sun Java HotSpot(TM) Client VM (build 1.4.2_12-b03, mixed mode)
------------------------------------------------
Comment 1 Martin Oberhuber CLA 2007-07-18 04:47:25 EDT
Created attachment 74035 [details]
Thread dump of the threads taking part in the deadlock
Comment 2 Martin Oberhuber CLA 2007-07-18 04:52:57 EDT
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.
Comment 3 Martin Oberhuber CLA 2007-07-18 04:58:38 EDT
Dave please review the patch for information, though I'm checking it in right away as [196919] Fix deadlock with workspace operations
Comment 4 David Dykstal CLA 2007-07-19 12:40:46 EDT
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.