Refresh the bundles after uninstalling #1253

jdneo · 2019-11-08T04:57:14Z

This PR add two things in loadBundles:

Refresh the uninstalled bundles before starting bundles
Avoid multiple singleton bundles installed

Why need this change

Scenario one

When a JDT.LS extension get updated, though the required bundle's version might be the same, but the location might be changed. Then the previous bundle get uninstalled, and the new bundle get installed. Now the framework will have both the uninstalled and installed bundles. When the bundle is declared as singleton, we will get an error when start the installed bundle.

Trigger the refresh action can avoid this exception

Scenario two

Two different extensions both want to install the same bundle with different version. If the bundle is declared as singleton, installing two different version of this bundle will cause exceptions.

So we can use a Collection to persist those installed singleton bundles.

Signed-off-by: Sheng Chen [email protected]

Signed-off-by: Sheng Chen <[email protected]>

fbricon · 2019-11-08T07:01:24Z

Tests fail on Jenkins

testforstephen · 2019-11-08T07:40:49Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

+				if (installedBundle != null) {
+					Module module = installedBundle.adapt(Module.class);
+					if (isSingleton(module.getCurrentRevision())) {
+						// We have installed this singleton bundle before, so skip this one.


Support installing two different versions of a non-singleton jar, make sense to me. But if two extensions try to install the same version of a non-singleton jar, will it succeed?

testforstephen · 2019-11-08T08:04:33Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

+			frameworkWiring.refreshBundles(toRefresh, new FrameworkListener() {
+				@Override
+				public void frameworkEvent(FrameworkEvent event) {
+					latch.countDown();


Need check the specific finish event such as PACKAGES_REFRESHED, ERROR, STOPPED ?

Yes, I should add event.getType() == FrameworkEvent.PACKAGES_REFRESHED check

jdneo · 2019-11-08T08:30:11Z

Might caused by the JUnit 5's required bundles are not installed in the test environment. I'm trying the build a new bundle for test purpose which has no required bundles.

Signed-off-by: Sheng Chen <[email protected]>

testforstephen · 2019-11-08T10:05:06Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

@@ -194,7 +215,9 @@ private static void refreshBundles(Set<Bundle> toRefresh, FrameworkWiring framew
 			frameworkWiring.refreshBundles(toRefresh, new FrameworkListener() {
 				@Override
 				public void frameworkEvent(FrameworkEvent event) {
-					latch.countDown();
+					if (event.getType() == FrameworkEvent.PACKAGES_REFRESHED) {


when it's FrameworkEvent.ERROR, also exit?

I tried to find if there is any example also handling other events when refresh the packages but with no success.

Is there any example I can refer to?

No example yet. i just read the API document of FrameworkListener, it says when there is any exception happening during refresh, it will report ERROR event.

I see. Then how about log an error message when this happens?

if we always are able to receive a FrameworkEvent.PACKAGES_REFRESHED event regardless of success or fail, i'm OK. My point is to make sure the latch.await is not blocking when there is bad status happening.

testforstephen · 2019-11-08T10:10:59Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

+					boolean shouldSkip = false;
+					for (Bundle bundle : installedBundleList) {
+						Module module = bundle.adapt(Module.class);
+						if (isSingleton(module.getCurrentRevision())) {


The singleton information can be resolved from the method getBundleInfo(String bundleLocation) directly, no need to get it from the module revision.

Do you mean getting it by parsing the symbol name? What if it contains whitespace?

this is a sample format Bundle-SymbolicName: org.eclipse.jdt.ls.core;singleton:=true in MANIFEST.MF. Currently before installing the new bundle, we already have logic to resolve the symbolic name and version info from the manifest file directly, why not bring the singleton attribute by the way.

Found a way to do it:

ManifestElement[] symbolicNameElements = ManifestElement.parseHeader(Constants.BUNDLE_SYMBOLICNAME, symbolicName); if (symbolicNameElements.length > 0) { symbolicName = symbolicNameElements[0].getValue(); String singleton = symbolicNameElements[0].getDirective(Constants.SINGLETON_DIRECTIVE); isSingleton = "true".equals(singleton); }

this is the existing utility method in BundleUtils. Looks like your sample code is better.

private static BundleInfo getBundleInfo(String bundleLocation) throws IOException { try (JarFile jarFile = new JarFile(bundleLocation)) { Manifest manifest = jarFile.getManifest(); if (manifest != null) { Attributes mainAttributes = manifest.getMainAttributes(); if (mainAttributes != null) { String bundleVersion = mainAttributes.getValue(Constants.BUNDLE_VERSION); if (StringUtils.isBlank(bundleVersion)) { return null; } String symbolicName = mainAttributes.getValue(Constants.BUNDLE_SYMBOLICNAME); if (StringUtils.isNotBlank(symbolicName) && symbolicName.indexOf(';') >= 0) { symbolicName = symbolicName.substring(0, symbolicName.indexOf(';')); } return new BundleInfo(bundleVersion, symbolicName); } } } return null; }

Signed-off-by: Sheng Chen <[email protected]>

jdneo · 2019-11-11T02:06:04Z

Code updated.

Signed-off-by: Sheng Chen <[email protected]>

testforstephen · 2019-11-11T05:38:09Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

+						latch.countDown();
+					} else if (event.getType() == FrameworkEvent.ERROR) {
+						latch.countDown();
+						JavaLanguageServerPlugin.logError("Error happens when refreshing the bundles");


carry the throwable info into the log.

Signed-off-by: Sheng Chen <[email protected]>

jdneo · 2019-11-11T06:27:28Z

test this please

testforstephen · 2019-11-11T06:41:44Z

org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/BundleUtilsTest.java

+			assertEquals(newBundle.getState(), Bundle.ACTIVE);
+		} finally {
+			// Uninstall the bundle to clean up the testing bundle context.
+			newBundle.uninstall();


BundleUtils.loadBundles(Arrays.asList(oldBundlePath, oldDependencyPath));
BundleUtils.loadBundles(Arrays.asList(newBundlePath, newDependencyPath));
probably throw uncaught exception, need extend the enclosing scope for try->finally.

Signed-off-by: Sheng Chen <[email protected]>

jdneo · 2019-11-12T09:27:11Z

@fbricon @testforstephen @snjeza Please let me know if there is any concern about this change.

fbricon

It's unclear to me what happens if 2 extensions are loaded, each of them having a dependency on the same bundle, same version, singleton or not, coming from 2 different urls

fbricon · 2019-11-12T09:40:35Z

org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/BundleUtilsTest.java

@@ -158,6 +160,187 @@ public void testLoadSameBundleMultipleTimes() throws Exception {
 		}
 	}

+	@Test
+	public void testLoadMultipleNonSongletonBundles() throws Exception {


testLoadMultipleNonSingletonBundles

snjeza · 2019-11-12T16:43:00Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

 				if (bundleInfo == null) {
 					status.add(new Status(IStatus.ERROR, context.getBundle().getSymbolicName(), "Failed to get bundleInfo for bundle from " + bundleLocation, null));
 					continue;
 				}
+
+				// Platform.getBundle() won't return the bundle with INSTALLED state, so we use this Map to persist the current installed bundles


you can try the following method:

private static Bundle getBundle(String symbolicName) { FrameworkWiring fwkWiring = JavaLanguageServerPlugin.getBundleContext().getBundle(0).adapt(FrameworkWiring.class); BundleContext context = fwkWiring.getBundle().getBundleContext(); if (Constants.SYSTEM_BUNDLE_SYMBOLICNAME.equals(symbolicName)) { symbolicName = context.getBundle(Constants.SYSTEM_BUNDLE_LOCATION).getSymbolicName(); } StringBuilder filter = new StringBuilder(); filter.append('(') .append(IdentityNamespace.IDENTITY_NAMESPACE) .append('=') .append(symbolicName) .append(')'); Map<String, String> directives = Collections.singletonMap(Namespace.REQUIREMENT_FILTER_DIRECTIVE, filter.toString()); Collection<BundleCapability> matchingBundleCapabilities = fwkWiring.findProviders(ModuleContainer.createRequirement(IdentityNamespace.IDENTITY_NAMESPACE, directives, Collections.emptyMap())); if (matchingBundleCapabilities.isEmpty()) { return null; } Bundle[] results = matchingBundleCapabilities.stream().map(c -> c.getRevision().getBundle()) // Remove all the bundles that are uninstalled .filter(bundle -> (bundle.getState() & (Bundle.UNINSTALLED)) == 0) .sorted((b1, b2) -> b2.getVersion().compareTo(b1.getVersion())) // highest version first .toArray(Bundle[]::new); return results.length > 0 ? results[0] : null; }

Thank you @snjeza, this method works!

Signed-off-by: Sheng Chen <[email protected]>

jdneo · 2019-11-13T04:17:02Z

test this please

jdneo · 2019-11-13T04:17:34Z

Summary of the current behavior:

Bundles with same symbolic name and same version will only be loaded with one instance, which is the first come one.
Non singleton bundles can be loaded with multiple instances as long as their versions are not the same
Singleton bundle will only be loaded with one instance, the last come one wins.

fbricon · 2019-11-13T12:00:56Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

+				if (bundles != null) {
+					boolean shouldSkip = false;
+					for (Bundle bundle : bundles) {
+						if (bundle.getVersion().equals(Version.parseVersion(bundleInfo.getVersion()))) {


please compute Version.parseVersion(bundleInfo.getVersion())) only once, before the loop

Signed-off-by: Sheng Chen <[email protected]>

fbricon · 2019-11-13T12:30:14Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BundleUtils.java

@@ -118,11 +118,12 @@ public static void loadBundles(Collection<String> bundleLocations) throws CoreEx
 					continue;
 				}

+				Version currentBundleVersion = Version.parseVersion(bundleInfo.getVersion());


move within if (bundles != null) block please

Signed-off-by: Sheng Chen <[email protected]>

Refresh the bundles after uninstalling

dcaa1a0

Signed-off-by: Sheng Chen <[email protected]>

jdneo force-pushed the cs/test-bundleload branch from e600cb5 to dcaa1a0 Compare November 8, 2019 05:47

testforstephen requested changes Nov 8, 2019

View reviewed changes

jdneo added 2 commits November 8, 2019 16:40

Use customized bundles to replace the Junit 5 runtime

29848f4

Signed-off-by: Sheng Chen <[email protected]>

Add version check for non-singleton bundle and framework event check

3a0d2f6

Signed-off-by: Sheng Chen <[email protected]>

testforstephen reviewed Nov 8, 2019

View reviewed changes

Use symbolInfo to check singleton

5d44847

Signed-off-by: Sheng Chen <[email protected]>

Add another test case when multiple extensions require the same bundle

a34f88a

Signed-off-by: Sheng Chen <[email protected]>

jdneo force-pushed the cs/test-bundleload branch from 4645b18 to a34f88a Compare November 11, 2019 05:27

testforstephen reviewed Nov 11, 2019

View reviewed changes

Log the throwable when error happens in refreshBundles

85ea54c

Signed-off-by: Sheng Chen <[email protected]>

testforstephen reviewed Nov 11, 2019

View reviewed changes

Wrap test cases with try-finally

633074a

Signed-off-by: Sheng Chen <[email protected]>

fbricon requested changes Nov 12, 2019

View reviewed changes

snjeza reviewed Nov 12, 2019

View reviewed changes

Use BundleCapability to get all the installed bundles in platform

34d5f95

Signed-off-by: Sheng Chen <[email protected]>

jdneo force-pushed the cs/test-bundleload branch from a82445b to 34d5f95 Compare November 13, 2019 03:10

fbricon requested changes Nov 13, 2019

View reviewed changes

Calculate bundle version outside the for-loop

cbe675d

Signed-off-by: Sheng Chen <[email protected]>

fbricon reviewed Nov 13, 2019

View reviewed changes

Move the version calculation into the if-block

a25e226

Signed-off-by: Sheng Chen <[email protected]>

fbricon merged commit 094fa48 into eclipse-jdtls:master Nov 13, 2019

fbricon added the enhancement label Nov 13, 2019

fbricon added this to the Mid November 2019 milestone Nov 13, 2019

jdneo deleted the cs/test-bundleload branch November 21, 2019 02:07

jdneo mentioned this pull request Nov 21, 2019

Add the location check back when loading bundles #1286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh the bundles after uninstalling #1253

Refresh the bundles after uninstalling #1253

jdneo commented Nov 8, 2019

fbricon commented Nov 8, 2019

testforstephen Nov 8, 2019

testforstephen Nov 8, 2019

jdneo Nov 8, 2019

jdneo commented Nov 8, 2019

testforstephen Nov 8, 2019

jdneo Nov 8, 2019

testforstephen Nov 8, 2019

jdneo Nov 8, 2019

testforstephen Nov 8, 2019

testforstephen Nov 8, 2019

jdneo Nov 8, 2019

testforstephen Nov 8, 2019 •

edited

Loading

jdneo Nov 8, 2019 •

edited

Loading

testforstephen Nov 8, 2019

jdneo commented Nov 11, 2019

testforstephen Nov 11, 2019

jdneo commented Nov 11, 2019

testforstephen Nov 11, 2019

jdneo commented Nov 12, 2019

fbricon left a comment

fbricon Nov 12, 2019

snjeza Nov 12, 2019

jdneo Nov 13, 2019

jdneo commented Nov 13, 2019

jdneo commented Nov 13, 2019 •

edited

Loading

fbricon Nov 13, 2019

fbricon Nov 13, 2019

jdneo Nov 13, 2019

Refresh the bundles after uninstalling #1253

Refresh the bundles after uninstalling #1253

Conversation

jdneo commented Nov 8, 2019

Why need this change

Scenario one

Scenario two

fbricon commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdneo commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

jdneo Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdneo commented Nov 11, 2019

Choose a reason for hiding this comment

jdneo commented Nov 11, 2019

Choose a reason for hiding this comment

jdneo commented Nov 12, 2019

fbricon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdneo commented Nov 13, 2019

jdneo commented Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Nov 8, 2019 •

edited

Loading

jdneo Nov 8, 2019 •

edited

Loading

jdneo commented Nov 13, 2019 •

edited

Loading