Bug 343182 - [region] check for bundle associated with another regionv20110421

4 files changed, 88 insertions, 70 deletions

diff --git a/bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/BundleIdBasedRegionTests.java b/bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/BundleIdBasedRegionTests.java
index 8ceeaaacc..b4327cf5a 100644
--- a/bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/BundleIdBasedRegionTests.java
+++ b/bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/BundleIdBasedRegionTests.java
@@ -57,6 +57,7 @@ public class BundleIdBasedRegionTests {
 	private RegionFilter mockRegionFilter;
 
 	private ThreadLocal<Region> threadLocal;
+	private Object regionAssociationMonitor = new Object();
 
 	@Before
 	public void setUp() throws Exception {
@@ -108,7 +109,7 @@ public class BundleIdBasedRegionTests {
 	public void testGetName() {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		assertEquals(REGION_NAME, r.getName());
 	}
 
@@ -139,7 +140,7 @@ public class BundleIdBasedRegionTests {
 		EasyMock.expect(this.mockGraph.getEdges(EasyMock.isA(Region.class))).andReturn(edges).anyTimes();
 		replayMocks();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 	}
 
@@ -147,7 +148,7 @@ public class BundleIdBasedRegionTests {
 	public void testAddExistingBundle() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 		r.addBundle(this.mockBundle);
 	}
@@ -162,13 +163,24 @@ public class BundleIdBasedRegionTests {
 		EasyMock.expect(mockBundle2.getBundleId()).andReturn(BUNDLE_ID_2).anyTimes();
 		EasyMock.replay(mockBundle2);
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 		r.addBundle(mockBundle2);
 	}
 
 	@Test(expected = BundleException.class)
-	public void testAddBundlePresentInAnotherRegion() throws BundleException {
+	public void testAddBundlePresentInAnotherRegion1() throws BundleException {
+		Region r = regionForBundlePersentInAnotherRegionTest();
+		r.addBundle(this.mockBundle);
+	}
+
+	@Test(expected = BundleException.class)
+	public void testAddBundlePresentInAnotherRegion2() throws BundleException {
+		Region r = regionForBundlePersentInAnotherRegionTest();
+		r.addBundle(this.mockBundle.getBundleId());
+	}
+
+	private Region regionForBundlePersentInAnotherRegionTest() {
 		this.regionIterator = new Iterator<Region>() {
 
 			private int next = 2;
@@ -194,13 +206,13 @@ public class BundleIdBasedRegionTests {
 		};
 		EasyMock.expect(this.mockGraph.iterator()).andReturn(this.regionIterator).anyTimes();
 		EasyMock.expect(this.mockGraph.getEdges(EasyMock.isA(Region.class))).andReturn(new HashSet<FilteredRegion>()).anyTimes();
-		EasyMock.expect(this.mockRegion.contains(EasyMock.eq(this.mockBundle))).andReturn(true).anyTimes();
-		EasyMock.expect(this.mockRegion2.contains(EasyMock.eq(this.mockBundle))).andReturn(false).anyTimes();
+		EasyMock.expect(this.mockRegion.contains(EasyMock.eq(BUNDLE_ID))).andReturn(true).anyTimes();
+		EasyMock.expect(this.mockRegion2.contains(EasyMock.eq(BUNDLE_ID))).andReturn(false).anyTimes();
 
 		replayMocks();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
-		r.addBundle(this.mockBundle);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
+		return r;
 	}
 
 	@Test
@@ -221,7 +233,7 @@ public class BundleIdBasedRegionTests {
 	public void testContains() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 		assertTrue(r.contains(this.mockBundle));
 	}
@@ -230,7 +242,7 @@ public class BundleIdBasedRegionTests {
 	public void testDoesNotContain() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		assertFalse(r.contains(this.mockBundle));
 	}
 
@@ -238,7 +250,7 @@ public class BundleIdBasedRegionTests {
 	public void testGetBundle() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 		assertEquals(this.mockBundle, r.getBundle(BUNDLE_SYMBOLIC_NAME, BUNDLE_VERSION));
 	}
@@ -247,7 +259,7 @@ public class BundleIdBasedRegionTests {
 	public void testGetBundleNotFound() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(this.mockBundle);
 		assertNull(r.getBundle(BUNDLE_SYMBOLIC_NAME_2, BUNDLE_VERSION));
 	}
@@ -256,7 +268,7 @@ public class BundleIdBasedRegionTests {
 	public void testConnectRegion() throws BundleException {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.connectRegion(this.mockRegion, this.mockRegionFilter);
 	}
 
@@ -264,8 +276,8 @@ public class BundleIdBasedRegionTests {
 	public void testEquals() {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
-		Region s = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
+		Region s = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		assertEquals(r, r);
 		assertEquals(r, s);
 		assertEquals(r.hashCode(), s.hashCode());
@@ -275,16 +287,16 @@ public class BundleIdBasedRegionTests {
 	public void testNotEqual() {
 		defaultSetUp();
 
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
-		Region s = new BundleIdBasedRegion(OTHER_REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
+		Region s = new BundleIdBasedRegion(OTHER_REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		assertFalse(r.equals(s));
 		assertFalse(r.equals(null));
 	}
 
 	@Test
-	public void testAddRemoveBundleId() {
+	public void testAddRemoveBundleId() throws BundleException {
 		defaultSetUp();
-		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal);
+		Region r = new BundleIdBasedRegion(REGION_NAME, this.mockGraph, this.mockBundleContext, this.threadLocal, this.regionAssociationMonitor);
 		r.addBundle(TEST_BUNDLE_ID);
 		assertTrue(r.contains(TEST_BUNDLE_ID));
 		r.removeBundle(TEST_BUNDLE_ID);
diff --git a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/BundleIdBasedRegion.java b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/BundleIdBasedRegion.java
index 8fe1f773c..edff17912 100644
--- a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/BundleIdBasedRegion.java
+++ b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/BundleIdBasedRegion.java
@@ -11,25 +11,16 @@
 
 package org.eclipse.equinox.internal.region;
 
-import org.eclipse.equinox.region.*;
-import org.eclipse.equinox.region.RegionDigraph.FilteredRegion;
-
-import java.util.concurrent.ConcurrentHashMap;
-
-import java.util.concurrent.ConcurrentMap;
-
-import java.io.File;
-import java.io.IOException;
-import java.io.InputStream;
+import java.io.*;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.HashSet;
 import java.util.Set;
-
-import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.BundleException;
-import org.osgi.framework.Version;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import org.eclipse.equinox.region.*;
+import org.eclipse.equinox.region.RegionDigraph.FilteredRegion;
+import org.osgi.framework.*;
 
 /**
  * {@link BundleIdBasedRegion} is an implementation of {@link Region} which keeps a track of the bundles in the region
@@ -50,9 +41,13 @@ final class BundleIdBasedRegion implements Region {
 	// A concurrent data structure ensures the contains method does not need synchronisation.
 	private final ConcurrentMap<Long, Boolean> bundleIds = new ConcurrentHashMap<Long, Boolean>();
 
-	// Updates do need synchronising to avoid races.
+	// This monitor guards bundleIds. bundleIds is a concurrent data structure and
+	// may be read without synchronisation, but updates need synchronising to avoid races.
 	private final Object updateMonitor = new Object();
 
+	// This monitor guards the check for bundle region association across a complete digraph.
+	private final Object regionAssociationMonitor;
+
 	private final String regionName;
 
 	private final RegionDigraph regionDigraph;
@@ -61,15 +56,18 @@ final class BundleIdBasedRegion implements Region {
 
 	private final ThreadLocal<Region> threadLocal;
 
-	BundleIdBasedRegion(String regionName, RegionDigraph regionDigraph, BundleContext bundleContext, ThreadLocal<Region> threadLocal) {
+	BundleIdBasedRegion(String regionName, RegionDigraph regionDigraph, BundleContext bundleContext, ThreadLocal<Region> threadLocal, Object regionAssociationMonitor) {
 		if (regionName == null)
 			throw new IllegalArgumentException("The region name must not be null");
 		if (regionDigraph == null)
 			throw new IllegalArgumentException("The region digraph must not be null");
+		if (regionAssociationMonitor == null)
+			throw new IllegalArgumentException("The region association monitor must not be null");
 		this.regionName = regionName;
 		this.regionDigraph = regionDigraph;
 		this.bundleContext = bundleContext;
 		this.threadLocal = threadLocal;
+		this.regionAssociationMonitor = regionAssociationMonitor;
 	}
 
 	/**
@@ -83,17 +81,17 @@ final class BundleIdBasedRegion implements Region {
 	 * {@inheritDoc}
 	 */
 	public void addBundle(Bundle bundle) throws BundleException {
-		synchronized (this.updateMonitor) {
-			checkBundleNotAssociatedWithAnotherRegion(bundle);
-
-			this.bundleIds.putIfAbsent(bundle.getBundleId(), Boolean.TRUE);
-		}
+		addBundle(bundle.getBundleId());
 	}
 
-	private void checkBundleNotAssociatedWithAnotherRegion(Bundle bundle) throws BundleException {
+	private void checkBundleNotAssociatedWithAnotherRegion(long bundleId) throws BundleException {
+		if (!Thread.holdsLock(regionAssociationMonitor)) {
+			throw new IllegalStateException("Must be holding the region association lock"); //$NON-NLS-1$
+		}
+		// note that obtaining the Iterator for the digraph requires locking the digraph monitor
 		for (Region r : this.regionDigraph) {
-			if (!this.equals(r) && r.contains(bundle)) {
-				throw new BundleException("Bundle '" + bundle + "' is already associated with region '" + r + "'", BundleException.INVALID_OPERATION);
+			if (!this.equals(r) && r.contains(bundleId)) {
+				throw new BundleException("Bundle '" + bundleId + "' is already associated with region '" + r + "'", BundleException.INVALID_OPERATION);
 			}
 		}
 	}
@@ -101,9 +99,16 @@ final class BundleIdBasedRegion implements Region {
 	/**
 	 * {@inheritDoc}
 	 */
-	public void addBundle(long bundleId) {
+	// There is a lock hierarchy between the per region update monitors, the per digraph
+	// regionAssociationMonitor followed by the digraph monitor:
+	// Any given thread acquires, at most, one updateMonitor followed by the
+	// regionAssociationMonitor followed by the digraph monitor
+	public void addBundle(long bundleId) throws BundleException {
 		synchronized (this.updateMonitor) {
-			this.bundleIds.putIfAbsent(bundleId, Boolean.TRUE);
+			synchronized (regionAssociationMonitor) {
+				checkBundleNotAssociatedWithAnotherRegion(bundleId);
+				this.bundleIds.putIfAbsent(bundleId, Boolean.TRUE);
+			}
 		}
 	}
 
@@ -199,6 +204,17 @@ final class BundleIdBasedRegion implements Region {
 		}
 	}
 
+	// The contains methods 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.
+	/**
+	 * {@inheritDoc}
+	 */
+	public boolean contains(long bundleId) {
+		return this.bundleIds.containsKey(bundleId);
+	}
+
 	/**
 	 * {@inheritDoc}
 	 */
@@ -230,13 +246,6 @@ final class BundleIdBasedRegion implements Region {
 	/**
 	 * {@inheritDoc}
 	 */
-	public boolean contains(long bundleId) {
-		return this.bundleIds.containsKey(bundleId);
-	}
-
-	/**
-	 * {@inheritDoc}
-	 */
 	public void removeBundle(Bundle bundle) {
 		removeBundle(bundle.getBundleId());
 
diff --git a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/StandardRegionDigraph.java b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/StandardRegionDigraph.java
index 74cd6f0cc..f3a7d2160 100644
--- a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/StandardRegionDigraph.java
+++ b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/internal/region/StandardRegionDigraph.java
@@ -11,9 +11,8 @@
 
 package org.eclipse.equinox.internal.region;
 
-import org.eclipse.equinox.region.*;
-
 import java.util.*;
+import org.eclipse.equinox.region.*;
 import org.osgi.framework.*;
 
 /**
@@ -30,6 +29,9 @@ public final class StandardRegionDigraph implements RegionDigraph {
 	private static final Set<FilteredRegion> EMPTY_EDGE_SET = Collections.unmodifiableSet(new HashSet<FilteredRegion>());
 
 	private final Object monitor = new Object();
+	// This monitor guards the check for bundle region association across a complete digraph.
+	// See BundleIdBaseRegion for more information
+	private final Object regionAssociationMonitor = new Object();
 
 	private final Set<Region> regions = new HashSet<Region>();
 
@@ -57,7 +59,7 @@ public final class StandardRegionDigraph implements RegionDigraph {
 	 * {@inheritDoc}
 	 */
 	public Region createRegion(String regionName) throws BundleException {
-		Region region = new BundleIdBasedRegion(regionName, this, this.bundleContext, this.threadLocal);
+		Region region = new BundleIdBasedRegion(regionName, this, this.bundleContext, this.threadLocal, this.regionAssociationMonitor);
 		synchronized (this.monitor) {
 			if (getRegion(regionName) != null) {
 				throw new BundleException("Region '" + regionName + "' already exists", BundleException.UNSUPPORTED_OPERATION);
diff --git a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/region/Region.java b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/region/Region.java
index c5dc5c21a..4164138ad 100644
--- a/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/region/Region.java
+++ b/bundles/org.eclipse.equinox.region/src/org/eclipse/equinox/region/Region.java
@@ -11,15 +11,10 @@
 
 package org.eclipse.equinox.region;
 
-import org.eclipse.equinox.region.RegionDigraph.FilteredRegion;
-
 import java.io.InputStream;
 import java.util.Set;
-
-import org.osgi.framework.Bundle;
-import org.osgi.framework.BundleContext;
-import org.osgi.framework.BundleException;
-import org.osgi.framework.Version;
+import org.eclipse.equinox.region.RegionDigraph.FilteredRegion;
+import org.osgi.framework.*;
 
 /**
  * A <i>region</i> is a subset of the bundles of an OSGi framework. A regions is "weakly" isolated from other regions
@@ -54,11 +49,7 @@ public interface Region {
 	 * <p>
 	 * If the bundle is already associated with another region, throws BundleException with exception type
 	 * INVALID_OPERATION.
-	 * <p>
-	 * 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.
-	 * 
+	 *
 	 * @param bundle the bundle to be associated with this region
 	 * @throws BundleException if the bundle cannot be associated with the region
 	 */
@@ -68,12 +59,16 @@ public interface Region {
 	 * Associates the given bundle id with this region. If the given bundle id is already associated with this region,
 	 * this is not an error and there is no effect.
 	 * <p>
+	 * If the bundle is already associated with another region, throws BundleException with exception type
+	 * INVALID_OPERATION.
+	 * <p>
 	 * This is useful when manipulating offline resolver states and bundle descriptions which do not correspond to
 	 * bundles.
 	 * 
 	 * @param bundleId the bundle id to be associated with this region
+	 * @throws BundleException if the bundle cannot be associated with the region
 	 */
-	void addBundle(long bundleId);
+	void addBundle(long bundleId) throws BundleException;
 
 	/**
 	 * Installs a bundle and associates the bundle with this region. The bundle's location will have the region name