RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened

[Alan Bateman]

On Wed, 14 Feb 2024 11:03:07 GMT, Christian Stein <cstein at openjdk.org> wrote:

> Please review this PR that makes the launcher helper keep a reference to the executable JAR file active after extracting the name of the main class and returning it as Class instance. Now, when loading classes from the JAR file, it hasn't to be re-opened.

So if we run today with `java -jar app.jar` then the JAR file will be opened, and its manifest parsed, by LauncherHelper.loadMainClass, then again by URLClassPath$JarLoader when it is lazily opened on the class path. That seems reasonable. It would of course be useful to see any performance/startup data but it might be hard to gather.

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 593:

> 591:     }
> 592: 
> 593:     private static String getMainClassFromJar(String jarname, JarFile jarFile) {

Can you check if the "jarname" parameter can be dropped, the error handling in this method should be able to use jarFile.getName().

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 596:

> 594:         String mainValue;
> 595:         try {
> 596:             Manifest manifest = jarFile.getManifest();

I think the try-catch around the block can be dropped and you can put a more targeted try-catch around getManifest, at least I think that is the only case remaining in getMainClassFromJar that needs error handling now.

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 823:

> 821:         // get the class name
> 822:         String cn;
> 823:         // store the jar file

"store the jar file" is a confusing comment to add. I think what you want to say is that the JarFile will put the underlying file in the JarFile/ZipFile cache and this will avoid needing to re-parse the manifest when the JAR file is opened on the class path, triggered by Class.forName.

src/java.base/share/classes/sun/launcher/LauncherHelper.java line 876:

> 874:                     jarFile.close();
> 875:                 } catch (IOException ioe) {
> 876:                     abort(ioe, "java.launcher.jar.error1", what);

java.launcher.jar.error1 is "Error: An unexpected error occurred while trying to open file". I can't think of any cases where it might fail but the error message would be confusing if it did.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1880069738
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489338851
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489340464
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489339435
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489342147