PDEState: unconditionally close stream

[jukzi]

For eager return on manifestStream == null jarFile was not closed.

[github-actions]

Test Results

     246 files  +       6       246 suites  +6   1h 11m 1s ⏱️ + 30m 40s
  3 287 tests  -        1    3 262 ✔️ ±       0  24 💤 ±  0  0 ❌  - 1  1 🔥 ±0 
10 158 runs  +2 721  10 085 ✔️ +2 698  72 💤 +24  0 ❌  - 1  1 🔥 ±0 

For more details on these errors, see this check.

Results for commit 617669a. ± Comparison against base commit 7dd409f.

This pull request removes 1 test.

AllPDEMinimalTests org.eclipse.pde.ui.tests.launcher.AllLauncherTests org.eclipse.pde.ui.tests.launcher.FeatureBasedLaunchTest ‑ Unknown test

♻️ This comment has been updated with latest results.

[HannesWell]

Looks good in general.
A few suggestion for further improvements below.

In a follow up, one could get rid of Hashtable/Dictionary here. It is far too often used in PDE in places were it is not necessary.

[HannesWell]

"Closing this ZIP file will close all of the input streams previously returned by invocations of the getInputStream".
So I think we can skip the nested try in this case. :)

[jukzi]

We probably could, but it's easier to read (also for static analysis tools) to just close any stream instead of looking into javadoc.

[HannesWell]

I think it would simplify the method if the processing of the input stream would be extraced into a separat method and then called from both locations.
Together with the other two suggestions the code would then be:

	private Dictionary<String, String> basicLoadManifest(File bundleLocation) {
		try {
			if ("jar".equalsIgnoreCase(IPath.fromOSString(bundleLocation.getName()).getFileExtension()) && bundleLocation.isFile()) { //$NON-NLS-1$
				try (ZipFile jarFile = new ZipFile(bundleLocation, ZipFile.OPEN_READ)) {
					ZipEntry manifestEntry = jarFile.getEntry(JarFile.MANIFEST_NAME);
					if (manifestEntry != null) {
						return readManifestEntries(jarFile.getInputStream(manifestEntry));
					}
				}
			} else {
				try (InputStream manifestStream = new FileInputStream(new File(bundleLocation, JarFile.MANIFEST_NAME))) {
					return readManifestEntries(manifestStream);
				}
			}
		} catch (IOException | BundleException e) {
			//ignore
		}

		//It is not a manifest, but a plugin or a fragment
		return null;
	}

	private Dictionary<String, String> readManifestEntries(InputStream manifestStream) throws IOException, BundleException {
		Hashtable<String, String> result = new Hashtable<>();
		result.putAll(ManifestElement.parseBundleManifest(manifestStream, null));
		return result;
	}

[jukzi]

The overhead of introducing an additional method is in this case bigger then the code reduction.

[jukzi]

ignoring random fail PlainJavaProjectTest #555