Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 359698

Summary: [prefs] Regression: EclipsePreferences#flush is subject to deadlock due to Open Call via ProjectPreferences / ProfilePreferences
Product: [Eclipse Project] Equinox Reporter: Martin Oberhuber <mober.at+eclipse>
Component: CompendiumAssignee: DJ Houghton <dj.houghton>
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: P3 CC: angvoz.dev, bakalsky, brian.vosburgh, dj.houghton, jamesblackburn+eclipse, john.arthorne, kaloyan, karenfbutzke, loskutov, malaperle, mober.at+eclipse, mschorn.eclipse, neil.hauge, remy.suen, stefan.dimov, Szymon.Brandys, tjwatson, wbprio
Version: 3.7.1   
Target Milestone: 3.7.2   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 333726    
Bug Blocks: 351231, 359485, 359851, 376141    

Description Martin Oberhuber CLA 2011-10-03 08:23:04 EDT 
+++ This bug was initially created as a clone of Bug #333726 +++

Build ID: Eclipse 3.7.1

The change from bug 333726 introduces a "synchronized" block for the
EclipsePreferences#flush() method. Bug 359485 has a detailed stack trace and analysis why this change causes CDT to deadlock. 

The problem is that from inside the (now synchronized) flush() method, a number of protected methods are called. These "open calls" bear the risk of deadlock. In this case, ProjectPreferences#save() extends EclipsePreferences, and from inside its save() method may perform a Thread switch to update the workspace, which may in turn call resource listeners. Those Thread switches occur with the synchronized block still holding a lock, causing CDT (and potentially others) to deadlock.

I think there are 2 problems with the current situation:

1. Given that there are Open Calls from inside the flush() method, a
   "synchronized" statement on method level is not appropriate (as any 
   book on Java Concurrency will confirm). A different way should be found
   to fix the problem from bug 333726.

2. ProjectPreferences extends EclipsePreferences, thus violating the API 
   boundary and "hiding" the fact that there are Open Calls. I'm wondering
   whether we could either (a) avoid that kind of non-API usage or 
   (b) improve comments or tooling to make it more apparent that protected
   method calls from EclipsePreferences are in fact Open Calls.

   In fact, ProfilePreferences in equinox.p2 has the same problem -- it also
   schedules a SaveJob from inside its save() method thus causing Thread
   Switch and potential deadlock.
Comment 1 Markus Schorn CLA 2011-10-04 05:35:04 EDT 
For clarification I'd like to rephrase the issue:

(1) A thread notifying a resource change can hold the lock on preferences:    
    CModelManager.resourceChanged(IResourceChangeEvent) line: 892    
    NotificationManager$1.run() line: 291    
    SafeRunner.run(ISafeRunnable) line: 42    
    NotificationManager.notify(...) line: 285    
    NotificationManager.broadcastChanges(...) line: 149    
    Workspace.broadcastPostChange() line: 395    
    Workspace.endOperation(...) line: 1530    
    Workspace.run(...) line: 2353    
    ProjectPreferences.save() line: 628    
    ProjectPreferences(EclipsePreferences).flush() line: 353         
    ProjectPreferences.flush() line: 380 

(2) The same lock is required when accessing a node in the preferences:
    ProjectPreferences(EclipsePreferences).getChild(...) line: 400
    ProjectPreferences(EclipsePreferences).internalNode(...) line: 542    
    ProjectPreferences(EclipsePreferences).node(String) line: 670    
 
With that CDT is lured into a deadlock:
(3) We use a lock X within the resource notification. 
(4) At a different place when owning X we access a node in the preferences. 

(1)+(3):  Locks preferences, locks X.
(4)+(2):  Locks X, locks preferences.
==> With a bad timing this dead-locks.
Comment 3 Martin Oberhuber CLA 2011-10-05 04:57:03 EDT 
CQ:WIND00309400

When importing the tree from the git R3_7_maintenance branch, I noticed that the bundle version has not been updated yet. It would be good to get the version updated for upcoming M-builds towards 3.7.2.
Comment 4 DJ Houghton CLA 2011-10-05 09:02:43 EDT 
Yep, there are a couple Equinox and p2 bundles that need incrementing. We had some issues with releasing things this week so I'm going to increment all the bundles at the same time once we get an accurate comparator result from the tests.
Comment 5 Andrey Loskutov CLA 2012-03-29 13:10:52 EDT 
Hi, it looks like the patch is NOT in 3.7.2, see bug 375601 comment 4
Comment 6 Marc-André Laperle CLA 2012-03-29 13:15:28 EDT 
Commit f81da88c2918758cb73695b3ebd3931670fcfb7a added the synchronize back and some subclasses call the internalFlush to prevent the dead lock but InstancePreferences can still deadlock.
Comment 7 Andrey Loskutov CLA 2012-03-29 13:19:36 EDT 
Ah, sorry, I mixed left/right sides.