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

Bug 376141

Summary:	[prefs] Regression: EclipsePreferences#flush is subject to deadlock due to Open Call via ProjectPreferences / ProfilePreferences
Product:	[Eclipse Project] Equinox	Reporter:	Markus Schorn <mschorn.eclipse>
Component:	Components	Assignee:	equinox.components-inbox <equinox.components-inbox>
Status:	CLOSED DUPLICATE	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, pwebster, remy.suen, stefan.dimov, Szymon.Brandys, tjwatson, wbprio
Version:	3.8.0 Juno
Target Milestone:	---
Hardware:	All
OS:	All
Whiteboard:
Bug Depends on:	333726, 359698
Bug Blocks:	351231, 359485, 359851

Description Markus Schorn

2012-04-05 02:56:16 EDT

The fix for the issue did not make it into eclipse 3.8M6.

+++ This bug was initially created as a clone of Bug #359698 +++

+++ 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 Andrey Loskutov

2012-04-05 03:24:05 EDT

(In reply to comment #0)
> 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.

Can you please consider to look into my recent analysis of the problem, documented in bug 333726 comment 27? This is also my conclusion and a possible solution path.

> 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.

Comments are nice but you hardly could expect that somebody would read through all javadocs and you can't even enforce that reader would do the right thing after reading. So if there is a better solution possible (as in point 1), then I would not spent any time on writing long comments how to workaround bad API.

Comment 2 John Arthorne

2012-04-05 13:14:55 EDT

This bug report is an exact clone of bug 359851 which was already fixed in Juno (3.8 and 4.2). It sounds from comment #1 that we are dealing with a different problem that Andrey has outlined in bug 333726. I have entered a new bug for this problem because I find dealing with a cloned copy of an old bug with an unrelated description too confusing. I have opened bug 376206 against equinox for the new problem.

*** This bug has been marked as a duplicate of bug 359851 ***