Bug 522313 - Remove loop used to acquire the bundle id lock on bundle
installation
When installing a new module there is a loop in the Storage class which
repeatedly attempts to acquire the next module ID lock before installing
the module into the module container.
This has always been a sort of hack because it is attempting to lock the
next module ID outside of the module container so that we can stage the
bundle content correctly into the bundle id folder before installing the
module into the container. To do that we needed to know what the next
module id is going to be and lock it so that no other thread attempts to
install over with the module ID we are trying to stage.
Instead of doing this strange locking loop this fix adds a new method to
the module container to get and increment the next module ID and then
passes that module ID in as part of the ModuleRevisionBuilder. This way
if multiple threads are installing modules they each will get thier own
unique module ID without requiring any outside the container locking.
Change-Id: I215a991bb4ef2a71eea542259b32e0c9509d9c8f
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
diff --git a/bundles/org.eclipse.osgi.tests/bundles_src/storage.hooks.a/org/eclipse/osgi/tests/hooks/framework/storage/a/TestHookConfigurator.java b/bundles/org.eclipse.osgi.tests/bundles_src/storage.hooks.a/org/eclipse/osgi/tests/hooks/framework/storage/a/TestHookConfigurator.java
index 57eb3e1..3f68e66 100644
--- a/bundles/org.eclipse.osgi.tests/bundles_src/storage.hooks.a/org/eclipse/osgi/tests/hooks/framework/storage/a/TestHookConfigurator.java
+++ b/bundles/org.eclipse.osgi.tests/bundles_src/storage.hooks.a/org/eclipse/osgi/tests/hooks/framework/storage/a/TestHookConfigurator.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2013, 2016 IBM Corporation and others.
+ * Copyright (c) 2013, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -63,6 +63,8 @@
public ModuleRevisionBuilder adaptModuleRevisionBuilder(ModuleEvent operation, Module origin, ModuleRevisionBuilder builder) {
if (TestHookConfigurator.replaceModuleBuilder) {
ModuleRevisionBuilder replace = new ModuleRevisionBuilder();
+ // try setting the ID to something which is checked during the test
+ replace.setId(5678);
replace.setSymbolicName("replace");
replace.setVersion(Version.parseVersion("1.1.1"));
replace.addCapability("replace", Collections.<String, String> emptyMap(), Collections.<String, Object> emptyMap());
@@ -71,6 +73,8 @@
return replace;
}
if (TestHookConfigurator.adaptManifest) {
+ // try setting the ID to something which is checked during the test
+ builder.setId(5678);
Map<String, String> dirs = Collections.emptyMap();
Map<String, Object> attrs = new HashMap<String, Object>();
attrs.put("test.file.path", getGeneration().getContent().getPath() + " - " + adaptCount.getAndIncrement());
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/AbstractTest.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/AbstractTest.java
index 846d8b9..2b06d17 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/AbstractTest.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/AbstractTest.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2013, 2016 IBM Corporation and others.
+ * Copyright (c) 2013, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -87,7 +87,14 @@
}
protected Module installDummyModule(Map<String, String> manifest, String location, ModuleContainer container) throws BundleException {
+ return installDummyModule(manifest, -1, location, container);
+ }
+
+ protected Module installDummyModule(Map<String, String> manifest, long id, String location, ModuleContainer container) throws BundleException {
ModuleRevisionBuilder builder = OSGiManifestBuilderFactory.createBuilder(manifest);
+ if (id > 0) {
+ builder.setId(id);
+ }
Module system = container.getModule(0);
return container.install(system, location, builder, null);
}
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
index 8849016..553819d 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
@@ -3274,6 +3274,45 @@
Assert.assertNull("Failed to resolve test.", report.getResolutionException());
}
+ @Test
+ public void testModuleIDSetting() throws BundleException, IOException {
+ DummyContainerAdaptor adaptor = createDummyAdaptor();
+ ModuleContainer container = adaptor.getContainer();
+
+ // install the system.bundle
+ Module systemBundle = installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container);
+ ResolutionReport report = container.resolve(Arrays.asList(systemBundle), true);
+ Assert.assertNull("Failed to resolve system.bundle.", report.getResolutionException());
+
+ Map<String, String> manifest = new HashMap<String, String>();
+
+ // test by installing bundles with decreasing IDs
+ List<Module> modules = new ArrayList<Module>();
+ for (int i = 5; i > 0; i--) {
+ manifest.clear();
+ manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+ manifest.put(Constants.BUNDLE_SYMBOLICNAME, String.valueOf(i));
+ modules.add(installDummyModule(manifest, i, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container));
+ }
+
+ // test that the modules have decreasing ID starting at 5
+ long id = 5;
+ for (Module module : modules) {
+ Assert.assertEquals("Wrong ID found.", id--, module.getId().longValue());
+ }
+
+ // test that error occurs when trying to use an existing ID
+ manifest.clear();
+ manifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+ manifest.put(Constants.BUNDLE_SYMBOLICNAME, String.valueOf("test.dup.id"));
+ try {
+ installDummyModule(manifest, 5, manifest.get(Constants.BUNDLE_SYMBOLICNAME), container);
+ fail("Expected to fail installation with duplicate ID.");
+ } catch (IllegalStateException e) {
+ // expected
+ }
+ }
+
private static void assertWires(List<ModuleWire> required, List<ModuleWire>... provided) {
for (ModuleWire requiredWire : required) {
for (List<ModuleWire> providedList : provided) {
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/hooks/framework/StorageHookTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/hooks/framework/StorageHookTests.java
index 67556d9..ad52a35 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/hooks/framework/StorageHookTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/hooks/framework/StorageHookTests.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2013, 2016 IBM Corporation and others.
+ * Copyright (c) 2013, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -147,6 +147,7 @@
installBundle();
Bundle b = framework.getBundleContext().getBundle(location);
+ assertNotEquals("Wrong ID.", 5678, b.getBundleId());
assertNotNull("Missing test bundle.", b);
List<Capability> testCaps = b.adapt(BundleRevision.class).getCapabilities("test.file.path");
assertEquals("Wrong number of test caps.", 1, testCaps.size());
@@ -158,6 +159,7 @@
assertEquals("Wrong origin", framework.getBundleContext().getBundle().getLocation(), location1);
b.update();
+ assertNotEquals("Wrong ID.", 5678, b.getBundleId());
testCaps = b.adapt(BundleRevision.class).getCapabilities("test.file.path");
assertEquals("Wrong number of test caps.", 1, testCaps.size());
String path2 = (String) testCaps.get(0).getAttributes().get("test.file.path");
@@ -176,6 +178,7 @@
framework = createFramework(configuration);
framework.start();
b = framework.getBundleContext().getBundle(location);
+ assertNotEquals("Wrong ID.", 5678, b.getBundleId());
testCaps = b.adapt(BundleRevision.class).getCapabilities("test.file.path");
assertEquals("Wrong number of test caps.", 1, testCaps.size());
path2 = (String) testCaps.get(0).getAttributes().get("test.file.path");
@@ -190,6 +193,7 @@
b.uninstall();
installBundle();
b = framework.getBundleContext().getBundle(location);
+ assertNotEquals("Wrong ID.", 5678, b.getBundleId());
assertNotNull("Missing test bundle.", b);
assertEquals("Wrong BSN.", "replace", b.getSymbolicName());
testCaps = b.adapt(BundleRevision.class).getCapabilities("replace");
diff --git a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
index 8447cd5..501cb69 100644
--- a/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.osgi/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@
Export-Package: org.eclipse.core.runtime.adaptor;x-friends:="org.eclipse.core.runtime",
org.eclipse.core.runtime.internal.adaptor;x-internal:=true,
org.eclipse.equinox.log;version="1.0";uses:="org.osgi.framework,org.osgi.service.log",
- org.eclipse.osgi.container;version="1.2";
+ org.eclipse.osgi.container;version="1.3";
uses:="org.eclipse.osgi.report.resolution,
org.osgi.framework.wiring,
org.osgi.framework.startlevel,
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
index 802b9f6..5756be2 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleContainer.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2012, 2016 IBM Corporation and others.
+ * Copyright (c) 2012, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -196,8 +196,11 @@
* @throws BundleException if some error occurs installing the module
*/
public Module install(Module origin, String location, ModuleRevisionBuilder builder, Object revisionInfo) throws BundleException {
+ long id = builder.getId();
ModuleRevisionBuilder adaptBuilder = getAdaptor().adaptModuleRevisionBuilder(ModuleEvent.INSTALLED, origin, builder, revisionInfo);
if (adaptBuilder != null) {
+ // be sure to restore the id from the original builder
+ adaptBuilder.setInternalId(id);
builder = adaptBuilder;
}
String name = builder.getSymbolicName();
@@ -288,8 +291,11 @@
* @throws BundleException if some error occurs updating the module
*/
public void update(Module module, ModuleRevisionBuilder builder, Object revisionInfo) throws BundleException {
+ long id = builder.getId();
ModuleRevisionBuilder adaptBuilder = getAdaptor().adaptModuleRevisionBuilder(ModuleEvent.UPDATED, module, builder, revisionInfo);
if (adaptBuilder != null) {
+ // be sure to restore the id from the original builder
+ adaptBuilder.setInternalId(id);
builder = adaptBuilder;
}
checkAdminPermission(module.getBundle(), AdminPermission.LIFECYCLE);
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
index 001e020..56518e7 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2012, 2016 IBM Corporation and others.
+ * Copyright (c) 2012, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -200,7 +200,14 @@
writeLock();
try {
int startlevel = Constants.SYSTEM_BUNDLE_LOCATION.equals(location) ? 0 : getInitialModuleStartLevel();
- long id = Constants.SYSTEM_BUNDLE_LOCATION.equals(location) ? 0 : getNextIdAndIncrement();
+ long id = Constants.SYSTEM_BUNDLE_LOCATION.equals(location) ? 0 : builder.getId();
+ if (id == -1) {
+ // the id is not set by the builder; get and increment the next ID
+ id = getAndIncrementNextId();
+ }
+ if (getModule(id) != null) {
+ throw new IllegalStateException("Duplicate module id: " + id + " used by module: " + getModule(id)); //$NON-NLS-1$//$NON-NLS-2$
+ }
EnumSet<Settings> settings = getActivationPolicySettings(builder);
Module module = load(location, builder, revisionInfo, id, settings, startlevel);
long currentTime = System.currentTimeMillis();
@@ -634,23 +641,13 @@
return moduleCycles;
}
- /**
- * Increments by one the next module ID
- */
- private long getNextIdAndIncrement() {
- // sanity check
- checkWrite();
- return nextId.getAndIncrement();
-
- }
-
private void checkWrite() {
if (monitor.getWriteHoldCount() == 0)
throw new IllegalMonitorStateException("Must hold the write lock."); //$NON-NLS-1$
}
/**
- * returns the next module ID
+ * returns the next module ID.
* <p>
* A read operation protected by the {@link #readLock() read} lock.
* @return the next module ID
@@ -665,6 +662,22 @@
}
/**
+ * Atomically increments by one the next module ID.
+ * <p>
+ * A write operation protected by the {@link #writeLock()} lock.
+ * @return the previous module ID
+ * @since 3.13
+ */
+ public final long getAndIncrementNextId() {
+ writeLock();
+ try {
+ return nextId.getAndIncrement();
+ } finally {
+ writeUnlock();
+ }
+ }
+
+ /**
* Returns the current timestamp for the revisions of this database.
* The timestamp is incremented any time a modification
* is made to the revisions in this database. For example:
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleRevisionBuilder.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleRevisionBuilder.java
index ebad630..412e537 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleRevisionBuilder.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleRevisionBuilder.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2012, 2016 IBM Corporation and others.
+ * Copyright (c) 2012, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -75,6 +75,7 @@
private int types = 0;
private final List<GenericInfo> capabilityInfos = new ArrayList<>();
private final List<GenericInfo> requirementInfos = new ArrayList<>();
+ private long id = -1;
/**
* Constructs a new module builder
@@ -108,6 +109,33 @@
}
/**
+ * Sets the module ID for the builder.
+ * <p>
+ * This module ID will be used if this builder is used to
+ * {@link ModuleContainer#install(Module, String, ModuleRevisionBuilder, Object) install}
+ * a module. If the ID is not set then a module ID will be generated by the module
+ * container at install time. If a module already exists with the specified ID
+ * then an error will occur when attempting to install a new module with this
+ * builder.
+ * <p>
+ * Note that the system module with location {@link Constants#SYSTEM_BUNDLE_LOCATION}
+ * always gets module ID of zero. The builder for the system module is not
+ * asked to provide the module ID for the system module at install time.
+ * @param id the module ID to use. Must be >= 1.
+ * @since 3.13
+ */
+ public void setId(long id) {
+ if (id < 1) {
+ throw new IllegalArgumentException("ID must be >=1."); //$NON-NLS-1$
+ }
+ this.id = id;
+ }
+
+ void setInternalId(long id) {
+ this.id = id;
+ }
+
+ /**
* Adds a capability to this builder using the specified namespace, directives and attributes
* @param namespace the namespace of the capability
* @param directives the directives of the capability
@@ -168,6 +196,18 @@
}
/**
+ * Returns the module id for this builder. A value of -1
+ * indicates that the module ID will be generated by the
+ * module container at {@link ModuleContainer#install(Module, String, ModuleRevisionBuilder, Object) install}
+ * time.
+ * @return the module id for this builder.
+ * @since 3.13
+ */
+ public long getId() {
+ return id;
+ }
+
+ /**
* Used by the container to build a new revision for a module.
* This builder is used to build a new {@link Module#getCurrentRevision() current}
* revision for the specified module.
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
index 85c9ecd..6aa42af 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/Storage.java
@@ -14,7 +14,6 @@
import java.net.*;
import java.security.*;
import java.util.*;
-import java.util.concurrent.TimeUnit;
import org.eclipse.core.runtime.adaptor.EclipseStarter;
import org.eclipse.osgi.container.*;
import org.eclipse.osgi.container.ModuleRevisionBuilder.GenericInfo;
@@ -530,20 +529,22 @@
boolean isReference = in instanceof ReferenceInputStream;
File staged = stageContent(in, sourceURL);
Generation generation = null;
- Long lockedID = getNextRootID();
try {
- BundleInfo info = new BundleInfo(this, lockedID, bundleLocation, 0);
+ Long nextID = moduleDatabase.getAndIncrementNextId();
+ BundleInfo info = new BundleInfo(this, nextID, bundleLocation, 0);
generation = info.createGeneration();
- File contentFile = getContentFile(staged, isReference, lockedID, generation.getGenerationId());
+ File contentFile = getContentFile(staged, isReference, nextID, generation.getGenerationId());
generation.setContent(contentFile, isReference);
// Check that we can open the bundle file
generation.getBundleFile().open();
setStorageHooks(generation);
ModuleRevisionBuilder builder = getBuilder(generation);
+ builder.setId(nextID);
+
Module m = moduleContainer.install(origin, bundleLocation, builder, generation);
- if (!lockedID.equals(m.getId())) {
+ if (!nextID.equals(m.getId())) {
// this revision is already installed. delete the generation
generation.delete();
return (Generation) m.getCurrentRevision().getRevisionInfo();
@@ -577,7 +578,6 @@
if (generation != null) {
generation.getBundleInfo().unlockGeneration(generation);
}
- idLocks.unlock(lockedID);
}
}
@@ -875,33 +875,6 @@
}
}
- private Long getNextRootID() throws BundleException {
- // Try up to 500 times
- for (int i = 0; i < 500; i++) {
- moduleDatabase.readLock();
- try {
- Long nextID = moduleDatabase.getNextId();
- try {
- if (idLocks.tryLock(nextID, 0, TimeUnit.SECONDS)) {
- return nextID;
- }
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new BundleException("Failed to obtain id locks for installation.", BundleException.STATECHANGE_ERROR, e); //$NON-NLS-1$
- }
- } finally {
- moduleDatabase.readUnlock();
- }
- // sleep to allow another thread to get the database lock
- try {
- Thread.sleep(50);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- }
- }
- throw new BundleException("Failed to obtain id locks for installation.", BundleException.STATECHANGE_ERROR); //$NON-NLS-1$
- }
-
/**
* Attempts to set the permissions of the file in a system dependent way.
* @param file the file to set the permissions on
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/MRUBundleFileList.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/MRUBundleFileList.java
index acdcf4f..e105c0d 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/MRUBundleFileList.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/storage/bundlefile/MRUBundleFileList.java
@@ -1,5 +1,5 @@
/*******************************************************************************
- * Copyright (c) 2005, 2016 IBM Corporation and others.
+ * Copyright (c) 2005, 2017 IBM Corporation and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -12,7 +12,9 @@
package org.eclipse.osgi.storage.bundlefile;
import java.io.IOException;
+import java.util.Collections;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
import org.eclipse.osgi.framework.eventmgr.*;
/**
@@ -40,14 +42,15 @@
// used to work around bug 275166
private boolean firstDispatch = true;
+ private AtomicInteger pending = new AtomicInteger();
+
public MRUBundleFileList(int fileLimit) {
// only enable the MRU if the initFileLimit is > MIN
this.fileLimit = fileLimit;
if (fileLimit >= MIN) {
this.bundleFileList = new BundleFile[fileLimit];
this.useStampList = new long[fileLimit];
- this.bundleFileCloser = new CopyOnWriteIdentityMap<>();
- this.bundleFileCloser.put(this, this);
+ this.bundleFileCloser = Collections.<Object, Object> singletonMap(this, this);
} else {
this.bundleFileList = null;
this.useStampList = null;
@@ -173,12 +176,22 @@
// TODO should log ??
} finally {
closingBundleFile.set(null);
+ pending.decrementAndGet();
}
}
private void closeBundleFile(BundleFile toRemove, EventManager manager) {
if (toRemove == null)
return;
+ int pendingNum = pending.incrementAndGet();
+ if (pendingNum > fileLimit) {
+ // delay to allow the closer to catchup
+ try {
+ Thread.sleep(Math.min(500, pendingNum));
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
try {
/* queue to hold set of listeners */
ListenerQueue<Object, Object, BundleFile> queue = new ListenerQueue<>(manager);
