Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PDEState: unconditionally close stream #635

Merged
merged 1 commit into from
Jul 10, 2023
Merged

PDEState: unconditionally close stream #635

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 14 additions & 30 deletions build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,45 +318,29 @@

//Return a dictionary representing a manifest. The data may result from plugin.xml conversion
private Dictionary<String, String> basicLoadManifest(File bundleLocation) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

InputStream manifestStream = null;
ZipFile jarFile = null;
try {
if ("jar".equalsIgnoreCase(IPath.fromOSString(bundleLocation.getName()).getFileExtension()) && bundleLocation.isFile()) { //$NON-NLS-1$
jarFile = new ZipFile(bundleLocation, ZipFile.OPEN_READ);
ZipEntry manifestEntry = jarFile.getEntry(JarFile.MANIFEST_NAME);
if (manifestEntry != null) {
manifestStream = jarFile.getInputStream(manifestEntry);
try (ZipFile jarFile = new ZipFile(bundleLocation, ZipFile.OPEN_READ)) {
ZipEntry manifestEntry = jarFile.getEntry(JarFile.MANIFEST_NAME);
if (manifestEntry != null) {
try (InputStream manifestStream = jarFile.getInputStream(manifestEntry)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (manifestStream != null) {
return new Hashtable<>(ManifestElement.parseBundleManifest(manifestStream, null));
}
}
}
}
} else {
manifestStream = new BufferedInputStream(new FileInputStream(new File(bundleLocation, JarFile.MANIFEST_NAME)));
try (InputStream manifestStream = new FileInputStream(new File(bundleLocation, JarFile.MANIFEST_NAME))) {
return new Hashtable<>(ManifestElement.parseBundleManifest(manifestStream, null));
}
}
} catch (IOException e) {
} catch (IOException | BundleException e) {
//ignore
}

//It is not a manifest, but a plugin or a fragment
if (manifestStream == null)
return null;

try {
Hashtable<String, String> result = new Hashtable<>();
result.putAll(ManifestElement.parseBundleManifest(manifestStream, null));
return result;
} catch (IOException | BundleException e) {
return null;
} finally {
try {
manifestStream.close();
} catch (IOException e1) {
//Ignore
}
try {
if (jarFile != null)
jarFile.close();
} catch (IOException e2) {
//Ignore
}
}
return null;
}

private boolean enforceSymbolicName(File bundleLocation, Dictionary<String, String> initialManifest) {
Expand Down Expand Up @@ -505,17 +489,17 @@
String release = environment.getProfileProperties().getProperty(JavaCore.COMPILER_COMPLIANCE);
try {
Collection<String> packages = new TreeSet<>();
String jrtPath = "lib/" + org.eclipse.jdt.internal.compiler.util.JRTUtil.JRT_FS_JAR; //$NON-NLS-1$

Check warning on line 492 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
String path = new File(vm.getInstallLocation(), jrtPath).toString(); // $NON-NLS-1$
var jrt = org.eclipse.jdt.internal.core.builder.ClasspathLocation.forJrtSystem(path, null, null, release);

Check warning on line 494 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
for (String moduleName : jrt.getModuleNames(null)) {

Check warning on line 495 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
var module = jrt.getModule(moduleName);

Check warning on line 496 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
if (module == null) {
continue;
}
for (var packageExport : module.exports()) {

Check warning on line 500 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
if (!packageExport.isQualified()) {

Check warning on line 501 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
packages.add(new String(packageExport.name()));

Check warning on line 502 in build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java

View check run for this annotation

Jenkins - eclipse-pde / Java Compiler

tycho-compiler:compile 

NORMAL:
}
}
}
Expand Down
Loading