Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v3]
On Mon, 19 Feb 2024 15:42:05 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Improve comment > - Update copyright year Re-tested Tier1-3 OK - PR Comment: https://git.openjdk.org/jdk/pull/17843#issuecomment-1953594654
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v3]
On Mon, 19 Feb 2024 15:42:05 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Improve comment > - Update copyright year Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1889544193
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v3]
On Thu, 15 Feb 2024 08:53:12 GMT, Alan Bateman wrote: >> `String jarname` is filled by the caller with the value of `String what`, >> and therefore contains the entire path to the executable JAR file. Using >> only `jarFile.getName()` will prevent any parent directories from being >> noted in error messages; which might affect some tests > >> `String jarname` is filled by the caller with the value of `String what`, >> and therefore contains the entire path to the executable JAR file. Using >> only `jarFile.getName()` will prevent any parent directories from being >> noted in error messages; which might affect some tests > > It returns the path name so I think it does what we want. Do you observe a > behavior difference? No, I don't. Also double-checked via `JarFile/ZipFile::getName`'s implementation returning the entire path to the JAR file. Re-running tier1-3 tests just in case... - PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1494762559
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v3]
On Mon, 19 Feb 2024 15:42:05 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Improve comment > - Update copyright year Marked as reviewed by coffeys (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1888733805
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v3]
> 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. Christian Stein has updated the pull request incrementally with two additional commits since the last revision: - Improve comment - Update copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17843/files - new: https://git.openjdk.org/jdk/pull/17843/files/202a4ef4..03d91efa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17843=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17843=01-02 Stats: 6 lines in 2 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17843/head:pull/17843 PR: https://git.openjdk.org/jdk/pull/17843
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Mon, 19 Feb 2024 07:32:31 GMT, Jaikiran Pai wrote: > Both the files will need a copyright year update. Right. Updating both files. > The JBS issue would need an appropriate noreg label too > https://openjdk.org/guide/#noreg Added `noreg-cleanup` to the issue. - PR Comment: https://git.openjdk.org/jdk/pull/17843#issuecomment-1952694603
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Mon, 19 Feb 2024 10:41:03 GMT, Alan Bateman wrote: >> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 821: >> >>> 819: // In LM_JAR mode, put the underlying file in the >>> JarFile/ZipFile cache. >>> 820: // This will avoid needing to re-parse the manifest when the >>> JAR file >>> 821: // is opened on the class path, triggered by Class.forName >>> below. >> >> Nice improvement to have. would it be ok to pad out this comment a bit more >> ? I found it difficult to understand the specifics. Perhaps : >> >> >> // In LM_JAR mode, keep the underlying file open to retain it >> in >> // the ZipFile HashMap cache. This will avoid needing to >> re-parse >> // the central directory when the file is opened on the >> class path, >> // triggered by Class.forName below. > >> Nice improvement to have. would it be ok to pad out this comment a bit more >> ? I found it difficult to understand the specifics. Perhaps : >> >> ``` >> // In LM_JAR mode, keep the underlying file open to retain it >> in >> // the ZipFile HashMap cache. This will avoid needing to >> re-parse >> // the central directory when the file is opened on the >> class path, >> // triggered by Class.forName below. >> ``` > > No issue with expanding the comment but I think leave it as "ZipFile cache" > as a more specific comment will quickly bit rot and confuse reader, and of > course the Launcher code doesn't care which collection is used internally in > the ZipFile code. I'll expand the comment as suggest, keeping the "JarFile/ZipFile cache" phrase. - PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1494722308
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Mon, 19 Feb 2024 10:30:24 GMT, Sean Coffey wrote: > Nice improvement to have. would it be ok to pad out this comment a bit more ? > I found it difficult to understand the specifics. Perhaps : > > ``` > // In LM_JAR mode, keep the underlying file open to retain it > in > // the ZipFile HashMap cache. This will avoid needing to > re-parse > // the central directory when the file is opened on the class > path, > // triggered by Class.forName below. > ``` No issue with expanding the comment but I think leave it as "ZipFile cache" as a more specific comment will quickly bit rot and confuse reader, and of course the Launcher code doesn't care which collection is used internally in the ZipFile code. - PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1494352861
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Thu, 15 Feb 2024 06:29:24 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove `String jarname` from `getMainClassFromJar`'s signature > - Introduce and use dedicated on-close error message src/java.base/share/classes/sun/launcher/LauncherHelper.java line 821: > 819: // In LM_JAR mode, put the underlying file in the > JarFile/ZipFile cache. > 820: // This will avoid needing to re-parse the manifest when the JAR > file > 821: // is opened on the class path, triggered by Class.forName below. Nice improvement to have. would it be ok to pad out this comment a bit more ? I found it difficult to understand the specifics. Perhaps : // In LM_JAR mode, keep the underlying file open to retain it in // the ZipFile HashMap cache. This will avoid needing to re-parse // the central directory when the file is opened on the class path, // triggered by Class.forName below. - PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1494339239
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Thu, 15 Feb 2024 06:29:24 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove `String jarname` from `getMainClassFromJar`'s signature > - Introduce and use dedicated on-close error message Hello Christian, the changes look OK to me. Both the files will need a copyright year update. The JBS issue would need an appropriate noreg label too https://openjdk.org/guide/#noreg - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1887725461
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Thu, 15 Feb 2024 06:29:24 GMT, Christian Stein 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. > > Christian Stein has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove `String jarname` from `getMainClassFromJar`'s signature > - Introduce and use dedicated on-close error message Updated version looks good to me. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17843#pullrequestreview-1882142536
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
On Wed, 14 Feb 2024 15:42:48 GMT, Christian Stein wrote: > `String jarname` is filled by the caller with the value of `String what`, and > therefore contains the entire path to the executable JAR file. Using only > `jarFile.getName()` will prevent any parent directories from being noted in > error messages; which might affect some tests It returns the path name so I think it does what we want. Do you observe a behavior difference? - PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1490650825
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened [v2]
> 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. Christian Stein has updated the pull request incrementally with two additional commits since the last revision: - Remove `String jarname` from `getMainClassFromJar`'s signature - Introduce and use dedicated on-close error message - Changes: - all: https://git.openjdk.org/jdk/pull/17843/files - new: https://git.openjdk.org/jdk/pull/17843/files/bd0927d3..202a4ef4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17843=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17843=00-01 Stats: 81 lines in 2 files changed: 18 ins; 18 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/17843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17843/head:pull/17843 PR: https://git.openjdk.org/jdk/pull/17843
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened
On Wed, 14 Feb 2024 15:49:01 GMT, Christian Stein wrote:
>> 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.
>
> Dropping that outer try-catch will lead to many whitespace-only changes due
> to un-indenting lines of the former block. Proceed or keep the indentation
> stable by making that internal method throw IOE and insert a comment-only
> block like:
>
>
> /* keep indentation stable */ {
> Manifest manifest = jarFile.getManifest();
> // ...
> }
> Dropping that outer try-catch will lead to many whitespace-only changes due
> to un-indenting lines of the former block. Proceed or keep the indentation
> stable by making that internal method throw IOE and insert a comment-only
> block like:
>
> ```java
> /* keep indentation stable */ {
> Manifest manifest = jarFile.getManifest();
> // ...
> }
> ```
I think it's okay to shift-tab to re-align it after you remove the try-catch.
It's a small patch and the changes are easy to see.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489736759
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened
On Wed, 14 Feb 2024 11:41:43 GMT, Alan Bateman 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.
>
> 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.
Dropping that outer try-catch will lead to many whitespace-only changes due to
un-indenting lines of the former block. Proceed or keep the indentation stable
by making that internal method throw IOE and insert a comment-only block like:
/* keep indentation stable */ {
Manifest manifest = jarFile.getManifest();
// ...
}
> 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.
Good catch! Introducing and using:
java.launcher.jar.error5=\
Error: An unexpected error occurred while trying to close file {0}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489697776
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489699548
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened
On Wed, 14 Feb 2024 11:40:16 GMT, Alan Bateman 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.
>
> 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().
`String jarname` is filled by the caller with the value of `String what`, and
therefore contains the entire path to the executable JAR file. Using only
`jarFile.getName()` will prevent any parent directories from being noted in
error messages; which might affect some tests
> 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.
Updating to use the precise description.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489688919
PR Review Comment: https://git.openjdk.org/jdk/pull/17843#discussion_r1489689891
Re: RFR: 8318812: LauncherHelper.checkAndLoadMain closes jar file that's about to be re-opened
On Wed, 14 Feb 2024 11:03:07 GMT, Christian Stein 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
