Community
Participate
Working Groups
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.
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.
Glyn could you give this a look. Do you have a better idea to solve this?
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".
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>
