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

Bug 343182

Summary:

[region] check for bundle associated with another region

Product:

[Eclipse Project] Equinox

Reporter:

Thomas Watson <tjwatson>

Component:

Components

Assignee:

Thomas Watson <tjwatson>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

glyn.normington

Version:

3.7

Target Milestone:

---

Hardware:

OS:

Mac OS X - Carbon (unsup.)

Whiteboard:

Attachments:

Description	Flags
patch and test add/changes	none

Description Thomas Watson

2011-04-18 13:49:33 EDT

There are two ways to add a bundle to a region, with a Bundle object or with the bundle id.

Currently the method BundleIdBasedRegion.addBundle(Bundle bundle) checks to make sure the bundle is not associated with another region before proceeding, but BundleIdBasedRegion.addBundle(long bundleId) does not.  I assume both methods should do the check?

Also there exists a race condition if more than one thread trying to add a bundle to multiple regions.  If the timing is correct the same bundle could get added to more than one region.  This is because a different updateMonitor object will be locked for each region while doing the consistency check on the digraph across all regions.

Comment 1 Thomas Watson

2011-04-19 14:17:36 EDT

Created attachment 193611 [details]
patch and test add/changes

This patch adds an additional monitor to lock while adding a bundle which must be held while adding a bundle and checking that the bundle is not associated with another region.

Comment 2 Thomas Watson

2011-04-19 14:19:19 EDT

Glyn could you give this a look.  Do you have a better idea to solve this?

Comment 3 Glyn Normington

2011-04-20 04:30:17 EDT

The fix looks good. I can't see any other way to resolve the race. Some extra comments would help though.

Please add a comment that there is a lock hierarchy between the per region updateMonitors, the per digraph regionAssociationMonitor, and the per digraph monitor: any given thread acquires at most one updateMonitor followed by the regionAssociationMonitor followed by the digraph monitor. Although the iteration in checkBundleNotAssociatedWithAnotherRegion causes an alien call (to StandardRegionDigraph), the lock hierarchy prevents deadlock.

It's probably also worth commenting that the contains method must not acquire the updateMonitor in order to prevent deadlock (since a thread holding the updateMonitor of one region can call checkBundleNotAssociatedWithAnotherRegion which can call the contains method on another region). This was true before this fix, but the existing comments aren't really sufficient. I think it is also worth changing the comment on the declaration of updateMonitor to:

"This monitor guards bundleIds. bundleIds is a concurrent data structure and may be read without synchronisation, but updates need synchronising to avoid races."

Please note there is a typo in "The region associaton monitor must not be null".

Comment 4 Thomas Watson

2011-04-20 10:40:22 EDT

I released the patch with some additional comments.  Thanks Glyn.

I also removed the following statement from the Region.addBundle(Bundle) javadoc which did not reflect the current behavior:

<javadoc>
If the bundle has the same bundle symbolic name and version as a bundle already present in the region or as a bundle import specified on a connection to another region, then BundleException with exception type DUPLICATE_BUNDLE_ERROR is thrown.
</javadoc>