Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 376206 - [prefs] EclipsePreferences creates deadlock risk by synchronizing on a public object
Summary: [prefs] EclipsePreferences creates deadlock risk by synchronizing on a public...
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Compendium (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows 7
: P3 critical (vote)
Target Milestone: Juno M7   Edit
Assignee: John Arthorne CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 243893 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-05 13:13 EDT by John Arthorne CLA
Modified: 2012-04-10 09:46 EDT (History)
7 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description John Arthorne CLA 2012-04-05 13:13:44 EDT 
EclipsePreferences synchronizes on "this" which is a public object used by clients. If client code also synchronizes on that object via synchronized(preferenceNode) it could create a deadlock. 

The complete analysis of this problem is in bug 333726 comment 27.
Comment 1 Sergey Prigogin CLA 2012-04-05 13:20:57 EDT 
Bug 333726 was critical and so is this one. The problem is not hypothetical. The deadlock does happen in CDT.
Comment 2 John Arthorne CLA 2012-04-09 13:09:45 EDT 
There are actually three problems here:

1) Use of a public object as the lock

2) Some accesses of the children field had no synchronization (internalChildNames)

3) None of the reads/writes on the properties field were synchronized. The data structure itself is thread safe, but lack of synchronization on field access/write could mean accessing stale values, or in the case of concurrent put(..) calls, it could result in lost preferences.
Comment 3 John Arthorne CLA 2012-04-09 13:12:10 EDT 
*** Bug 243893 has been marked as a duplicate of this bug. ***
Comment 5 Andrey Loskutov CLA 2012-04-10 03:51:44 EDT 
(In reply to comment #4)
> http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=087d3b0910013b190998a81e3d49f2b99f7f1e34

Hi John,
while backporting your patch to 3.7.2 code I found one minor issue: shareStrings(StringPool pool) has an unsynchronized read of "properties" field value:

public void shareStrings(StringPool pool) {
	properties.shareStrings(pool);

Shouldn't be:

public void shareStrings(StringPool pool) {
	synchronized (childAndPropertyLock) {
		properties.shareStrings(pool);
	}

instead?
Comment 6 Andrey Loskutov CLA 2012-04-10 03:55:52 EDT 
(In reply to comment #5)

> Shouldn't be:
> 
> public void shareStrings(StringPool pool) {
>     synchronized (childAndPropertyLock) {
>         properties.shareStrings(pool);
>     }
> 
> instead?

Or better:

public void shareStrings(StringPool pool) {
	//thread safety: copy reference in case of concurrent change
	ImmutableMap temp;
	synchronized (childAndPropertyLock) {
		temp = properties;
	}
	temp.shareStrings(pool);

?
Comment 7 John Arthorne CLA 2012-04-10 09:46:06 EDT 
(In reply to comment #6)
> 
> public void shareStrings(StringPool pool) {
>     //thread safety: copy reference in case of concurrent change
>     ImmutableMap temp;
>     synchronized (childAndPropertyLock) {
>         temp = properties;
>     }
>     temp.shareStrings(pool);
> 
> ?

I didn't bother with that one because it's just an optimization and having a stale value wouldn't cause any harm. But we might as well be consistent so I made this change:

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=52a7f6d48eacc72506ba42a6216246d2b4e441e8