Bug 576636 - Fix issues with added dynamic imports and multi-thread

resolution

If one thread is calling
org.eclipse.osgi.internal.loader.BundleLoader.addDynamicImportPackage(ManifestElement[])
while other threads are trying to perform a resolution, dynamic or
otherwise then there is a timing issue that could prevent dynamic
resolution to the packages added with
BundleLoader.addDynamicImportPackage.  The
BundleLoader.addDynamicImportPackage gets used by WovenClassImpl to
allow weavers to add dynamic imports while they are weaving a class.

The issue is the ModuleWiring.requirements field may get overwritten by
a resolve operation in another thread doing a resolver operation after
BundleLoader.addDynamicImportPackage has been called.  This is because a
resolve operation happens on a snapshot of the wirings while holding a
read lock. Then the resolve operation happens with no locks because it
is done on copies of the live objects.  During this window, before the
resolution delta is applied, another thead could call
BundleLoader.addDynamicImportPackage which modifies the
ModuleWiring.requirements field.

Finally the resolution thread finds a resolution result and attempts to
apply it to the current wirings.  At this point it checks the timestamps
of the moduledatabase and finds it is safe to apply the results.  These
results may end up replacing the ModuleWiring.requirements of a wiring
with stale data (effectively removing the dynamically added imports.
The result is that a dynamic import resolution will then fail later.

The fix is to have the
org.eclipse.osgi.container.ModuleWiring.addDynamicImports(ModuleRevisionBuilder)
method update the ModuleDatabase timestamp such that any in progress
resolution operations can detect they are using stale data and cause
them to retry with another current snapshot.

Change-Id: I97aa78193d49d27088edb66189b2daccd29e8b06
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.framework/+/186513
Tested-by: Equinox Bot <equinox-bot@eclipse.org>

7 files changed, 225 insertions, 9 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/AddDynamicImportTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/AddDynamicImportTests.java
new file mode 100644
index 000000000..447cae9c2
--- /dev/null
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/AddDynamicImportTests.java
@@ -0,0 +1,183 @@
+/*******************************************************************************
+ * Copyright (c) 2018, 2020 IBM Corporation and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.osgi.tests.bundles;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.BiConsumer;
+import junit.framework.Test;
+import junit.framework.TestSuite;
+import org.eclipse.osgi.container.Module;
+import org.eclipse.osgi.internal.framework.EquinoxConfiguration;
+import org.eclipse.osgi.internal.loader.BundleLoader;
+import org.eclipse.osgi.internal.loader.ModuleClassLoader;
+import org.eclipse.osgi.launch.Equinox;
+import org.eclipse.osgi.tests.OSGiTestsActivator;
+import org.eclipse.osgi.util.ManifestElement;
+import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.BundleException;
+import org.osgi.framework.Constants;
+import org.osgi.framework.namespace.PackageNamespace;
+import org.osgi.framework.wiring.BundleWiring;
+
+public class AddDynamicImportTests extends AbstractBundleTests {
+
+
+	public static Test suite() {
+		return new TestSuite(AddDynamicImportTests.class);
+	}
+
+
+	public void testAddDynamicImportMultipleTimes() throws Exception {
+		runTest((a, b) -> {
+			for (int i = 0; i < 1000; i++) {
+				BundleLoader bl = ((ModuleClassLoader) b.adapt(BundleWiring.class).getClassLoader()).getBundleLoader();
+				Map<BundleLoader, Boolean> addedDynamicImport = new ConcurrentHashMap<>();
+
+				List<Runnable> runs = new ArrayList<>();
+				Runnable test = () -> {
+					addedDynamicImport.computeIfAbsent(bl, (bl2) -> {
+						// this should clear the miss cache
+						bl2.addDynamicImportPackage(createImports("org.osgi.framework"));
+						return true;
+					});
+					try {
+						b.loadClass("org.osgi.framework.Bundle");
+					} catch (ClassNotFoundException e) {
+						fail("Should find class.", e);
+					}
+				};
+
+				// test with three threads in a parallel stream
+				runs.add(test);
+				runs.add(test);
+				runs.add(test);
+
+				runs.parallelStream().forEach(Runnable::run);
+
+				Module mb = bl.getWiring().getRevision().getRevisions().getModule();
+				mb.getContainer().refresh(Collections.singletonList(mb));
+			}
+		});
+	}
+
+	public void testAddDynamicImportWhileDynamicWiring() throws Exception {
+		runTest((a, b) -> {
+
+			for (int i = 0; i < 1000; i++) {
+				System.out.println("Doing " + i);
+				BundleLoader bl = ((ModuleClassLoader) b.adapt(BundleWiring.class).getClassLoader()).getBundleLoader();
+				List<Runnable> runs = new ArrayList<>();
+				Runnable testAddDynamic = () -> {
+					// this should clear the miss cache
+					bl.addDynamicImportPackage(createImports("org.osgi.framework"));
+					try {
+						b.loadClass("org.osgi.framework.Bundle");
+					} catch (ClassNotFoundException e) {
+						fail("Should find class.", e);
+					}
+				};
+
+				AtomicInteger pkgNumber = new AtomicInteger(1);
+				Runnable testResolveDynamic = () -> {
+					a.getResource("test/export/pkg" + pkgNumber.getAndAdd(1) + "/DoesNotExist.txt");
+				};
+
+				// test with three threads in a parallel stream
+				runs.add(testAddDynamic);
+				runs.add(testResolveDynamic);
+				runs.add(testResolveDynamic);
+				runs.add(testResolveDynamic);
+				runs.add(testResolveDynamic);
+
+				runs.parallelStream().forEach(Runnable::run);
+
+				assertEquals("Wrong number of required wires for wiring A", 4,
+						a.adapt(BundleWiring.class).getRequiredWires(PackageNamespace.PACKAGE_NAMESPACE).size());
+				Module m = bl.getWiring().getRevision().getRevisions().getModule();
+				m.getContainer().refresh(Collections.singletonList(m));
+			}
+		});
+	}
+
+	ManifestElement[] createImports(String packages) {
+		try {
+			return ManifestElement.parseHeader(Constants.DYNAMICIMPORT_PACKAGE, packages);
+		} catch (BundleException e) {
+			fail("Unexpected Exception", e);
+			return null;
+		}
+	}
+
+	private void runTest(BiConsumer<Bundle, Bundle> testConsumer) {
+		File config = OSGiTestsActivator.getContext().getDataFile(getName());
+
+		Map<String, String> headersA = new HashMap<>();
+		headersA.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+		headersA.put(Constants.BUNDLE_SYMBOLICNAME, getName() + "A");
+		headersA.put(Constants.DYNAMICIMPORT_PACKAGE, "test.export.*");
+
+		Map<String, String> headersB = new HashMap<>();
+		headersB.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+		headersB.put(Constants.BUNDLE_SYMBOLICNAME, getName() + "B");
+		headersB.put(Constants.EXPORT_PACKAGE,
+				"test.export.pkg1, test.export.pkg2, test.export.pkg3, test.export.pkg4");
+
+		config.mkdirs();
+
+		try {
+			File bundleAFile = SystemBundleTests.createBundle(config, getName() + "A", headersA);
+			File bundleBFile = SystemBundleTests.createBundle(config, getName(), headersB);
+
+			Map<String, String> fwkConfig = new HashMap<>();
+			fwkConfig.put(Constants.FRAMEWORK_STORAGE, config.getAbsolutePath());
+			fwkConfig.put(EquinoxConfiguration.PROP_RESOLVER_BATCH_TIMEOUT, "10000000");
+			Equinox equinox = new Equinox(fwkConfig);
+			try {
+				equinox.start();
+				BundleContext systemContext = equinox.getBundleContext();
+
+				Bundle bundleA = systemContext.installBundle(bundleAFile.toURI().toString());
+				bundleA.start();
+
+				Bundle bundleB = systemContext.installBundle(bundleBFile.toURI().toString());
+				bundleB.start();
+
+				// load the class first will cause a miss cache to be added.
+				try {
+					bundleA.loadClass("org.osgi.framework.Bundle");
+					fail("Expected to fail class load.");
+				} catch (ClassNotFoundException e) {
+					// expected
+				}
+				testConsumer.accept(bundleA, bundleB);
+			} catch (BundleException be) {
+				fail("Unexpected exception.", be);
+			} finally {
+				stopQuietly(equinox);
+			}
+		} catch (IOException ioe) {
+			fail("Unexpected exception", ioe);
+		}
+
+	}
+}
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java
index 9805426ab..747dce999 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/bundles/BundleTests.java
@@ -39,6 +39,7 @@ public class BundleTests {
 		suite.addTest(NativeCodeBundleTests.suite());
 		suite.addTest(PlatformAdminBundleTests.suite());
 		suite.addTest(ListenerTests.suite());
+		suite.addTest(AddDynamicImportTests.suite());
 		return suite;
 	}
 }
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 a22e7740d..77f8e09ed 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
@@ -545,6 +545,26 @@ public class ModuleDatabase {
 		}
 	}
 
+	
+	/**
+	 * Perform the specified operation while holding the write lock.
+	 * This will also increment the timestamps and optionally the
+	 * revisions timestamps.
+	 * <p>
+	 * A write operation protected by the {@link #writeLock() write} lock.
+	 * @param incrementRevision if true the revision timestamps will be incremented after successfully running the operation
+	 * @param op the operation to run while holding the write lock.
+	 */
+	final void writeLockOperation(boolean incrementRevision, Runnable op) {
+		writeLock();
+		try {
+			op.run();
+			incrementTimestamps(incrementRevision);
+		} finally {
+			writeUnlock();
+		}
+	}
+
 	/**
 	 * Returns a snapshot of all modules ordered by module ID.
 	 * <p>
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleResolver.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleResolver.java
index b42a993a8..37b6c0dc9 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleResolver.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleResolver.java
@@ -1317,7 +1317,8 @@ final class ModuleResolver {
 		}
 
 		private Map<Resource, List<Wire>> resolveDynamic() throws ResolutionException {
-			return new ResolverImpl(new Logger(0), null).resolveDynamic(this, dynamicReq.getRevision().getWiring(), dynamicReq.getOriginal());
+			return new ResolverImpl(new Logger(0), null).resolveDynamic(this, wirings.get(dynamicReq.getResource()),
+					dynamicReq.getOriginal());
 		}
 
 		private void filterResolvable() {
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleWiring.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleWiring.java
index 4c46bab0b..bd52a1db2 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleWiring.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleWiring.java
@@ -422,14 +422,21 @@ public final class ModuleWiring implements BundleWiring {
 		}, NamespaceList.REQUIREMENT);
 
 		ModuleDatabase moduleDatabase = revision.getRevisions().getContainer().moduleDatabase;
-		moduleDatabase.writeLock();
-		try {
+		// Use the writeLockOperation to atomically update the revision timestamps
+		// This is necessary to make sure any in flight resolve operations are using the
+		// latest wiring data and avoids them overwriting the requirements incorrectly.
+		moduleDatabase.writeLockOperation(true, () -> {
 			NamespaceList.Builder<ModuleRequirement> requirmentsBuilder = requirements.createBuilder();
 			requirmentsBuilder.addAll(newRequirements);
 			requirements = requirmentsBuilder.build();
-		} finally {
-			moduleDatabase.writeUnlock();
-		}
+			// clear out miss cache when adding new dynamic imports.
+			dynamicMissRef.updateAndGet((s) -> {
+				if (s != null) {
+					s.clear();
+				}
+				return s;
+			});
+		});
 	}
 
 	void addDynamicPackageMiss(String packageName) {
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/InternalUtils.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/InternalUtils.java
index 08ac1c32a..58e02050f 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/InternalUtils.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/container/InternalUtils.java
@@ -48,6 +48,9 @@ public class InternalUtils {
 	 * @return an effectively mutable and lazy copy of the given list.
 	 */
 	public static <T> List<T> asCopy(List<? extends T> list) {
+		if (list == null) {
+			return null;
+		}
 		if (!(list instanceof RandomAccess)) {
 			throw new IllegalArgumentException("Only RandomAccess lists are supported"); //$NON-NLS-1$
 		}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
index e966a8229..0e5767558 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/loader/BundleLoader.java
@@ -1096,8 +1096,6 @@ public class BundleLoader extends ModuleLoader {
 		}
 
 		if (dynamicImports.size() > 0) {
-			addDynamicImportPackage(dynamicImports.toArray(new String[dynamicImports.size()]));
-
 			Map<String, String> dynamicImportMap = new HashMap<>();
 			dynamicImportMap.put(Constants.DYNAMICIMPORT_PACKAGE, importSpec.toString());
 
@@ -1107,7 +1105,10 @@ public class BundleLoader extends ModuleLoader {
 			} catch (BundleException e) {
 				throw new RuntimeException(e);
 			}
-
+			// Add the dynamic imports to the loader second to be sure the requirement
+			// gets added to the wiring first. This avoids issues if another
+			// thread tries to dynamic resolve before all is done here.
+			addDynamicImportPackage(dynamicImports.toArray(new String[dynamicImports.size()]));
 		}
 	}