Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Tue, 16 Apr 2024 13:26:43 GMT, Jan Lahoda wrote:
>> src/java.base/share/native/libjli/java.c line 419:
>>
>>> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
>>> mainArgs) {
>>> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass,
>>> "", "()V");
>>> 419: CHECK_EXCEPTION_FAIL();
>>
>> Shouldn't this also not be checked until you know you have an instance
>> method?
>
> You mean to first check whether the method exists, and only then try to
> lookup the constructor? Not sure if that has observable effects, but I can do
> that.
>
> I missed [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581) before,
> my plan is to adjust this "check for exception and clear it" check to only
> work for `NoSuchMethodError`, and let the exception pass for any other
> exception.
Yes that is what I meant. If the main method is static then the class need not
have any such constructor (unless that is part of the spec for this
functionality?).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1568370439
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Tue, 16 Apr 2024 10:03:21 GMT, David Holmes wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review feedback.
>
> src/java.base/share/native/libjli/java.c line 419:
>
>> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
>> mainArgs) {
>> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass,
>> "", "()V");
>> 419: CHECK_EXCEPTION_FAIL();
>
> Shouldn't this also not be checked until you know you have an instance method?
You mean to first check whether the method exists, and only then try to lookup
the constructor? Not sure if that has observable effects, but I can do that.
I missed [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581) before, my
plan is to adjust this "check for exception and clear it" check to only work
for `NoSuchMethodError`, and let the exception pass for any other exception.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1567361164
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Mon, 15 Apr 2024 07:36:05 GMT, Jan Lahoda wrote:
>> Consider code like:
>>
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>> and compile and run it, with preview enabled, like:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>>
>>
>> That is wrong, as the `main` method is static, and there is no need to
>> create a new instance of the class.
>>
>> The reason is that as launcher attempts to invoke the main method, it goes
>> in the following order: 1) static-main-with-args; 2)
>> instance-main-with-args; 3) static-main-without-args; 4)
>> instance-main-without-args. But, for the instance variants, the code first
>> creates a new instance of the given class, and only then attempts to lookup
>> the `main` method, and will pass to the next option when the `main` method
>> lookup fails. So, when invoking static-main-without-args, the current class
>> instance may be created for instance-main-with-args, which will then fail
>> due to the missing `main(String[])` method.
>>
>> The proposed solution to this problem is to simply first do a lookup for the
>> `main` method (skipping to the next variant when the given main method does
>> not exist, without instantiating the current class).
>>
>> There is also a relatively closely related problem: what happens when the
>> constructor throws an exception?
>>
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>>
>> when compiled an run, this produces no output whatsoever:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>>
>>
>> This is because any exceptions thrown from the constructor are effectively
>> ignored, and the launcher will continue with the next variant. This seems
>> wrong - the exception should be printed for the user, like:
>>
>> $ java --enable-preview -classpath /tmp/ MainClass
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>>
>>
>> This patch proposes to do that by not consuming the exceptions thrown from
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
src/java.base/share/native/libjli/java.c line 419:
> 417: invokeInstanceMainWithArgs(JNIEnv *env, jclass mainClass, jobjectArray
> mainArgs) {
> 418: jmethodID constructor = (*env)->GetMethodID(env, mainClass,
> "", "()V");
> 419: CHECK_EXCEPTION_FAIL();
Shouldn't this also not be checked until you know you have an instance method?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1567092479
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Mon, 15 Apr 2024 07:36:05 GMT, Jan Lahoda wrote:
>> Consider code like:
>>
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>> and compile and run it, with preview enabled, like:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>>
>>
>> That is wrong, as the `main` method is static, and there is no need to
>> create a new instance of the class.
>>
>> The reason is that as launcher attempts to invoke the main method, it goes
>> in the following order: 1) static-main-with-args; 2)
>> instance-main-with-args; 3) static-main-without-args; 4)
>> instance-main-without-args. But, for the instance variants, the code first
>> creates a new instance of the given class, and only then attempts to lookup
>> the `main` method, and will pass to the next option when the `main` method
>> lookup fails. So, when invoking static-main-without-args, the current class
>> instance may be created for instance-main-with-args, which will then fail
>> due to the missing `main(String[])` method.
>>
>> The proposed solution to this problem is to simply first do a lookup for the
>> `main` method (skipping to the next variant when the given main method does
>> not exist, without instantiating the current class).
>>
>> There is also a relatively closely related problem: what happens when the
>> constructor throws an exception?
>>
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>>
>> when compiled an run, this produces no output whatsoever:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>>
>>
>> This is because any exceptions thrown from the constructor are effectively
>> ignored, and the launcher will continue with the next variant. This seems
>> wrong - the exception should be printed for the user, like:
>>
>> $ java --enable-preview -classpath /tmp/ MainClass
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>>
>>
>> This patch proposes to do that by not consuming the exceptions thrown from
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
Does this also fix [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581)?
-
PR Comment: https://git.openjdk.org/jdk/pull/18753#issuecomment-2057206293
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Mon, 15 Apr 2024 07:36:05 GMT, Jan Lahoda wrote:
>> Consider code like:
>>
>> public class MainClass {
>> public MainClass() {
>> System.out.println("Constructor called!");
>> }
>> public static void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>> and compile and run it, with preview enabled, like:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> Constructor called!
>> main called!
>>
>>
>> That is wrong, as the `main` method is static, and there is no need to
>> create a new instance of the class.
>>
>> The reason is that as launcher attempts to invoke the main method, it goes
>> in the following order: 1) static-main-with-args; 2)
>> instance-main-with-args; 3) static-main-without-args; 4)
>> instance-main-without-args. But, for the instance variants, the code first
>> creates a new instance of the given class, and only then attempts to lookup
>> the `main` method, and will pass to the next option when the `main` method
>> lookup fails. So, when invoking static-main-without-args, the current class
>> instance may be created for instance-main-with-args, which will then fail
>> due to the missing `main(String[])` method.
>>
>> The proposed solution to this problem is to simply first do a lookup for the
>> `main` method (skipping to the next variant when the given main method does
>> not exist, without instantiating the current class).
>>
>> There is also a relatively closely related problem: what happens when the
>> constructor throws an exception?
>>
>> public class MainClass {
>> public MainClass() {
>> if (true) throw new RuntimeException();
>> }
>> public void main() {
>> System.out.println("main called!");
>> }
>> }
>>
>>
>> when compiled an run, this produces no output whatsoever:
>>
>> $ javac /tmp/MainClass.java
>> $ java --enable-preview -classpath /tmp MainClass
>> $
>>
>>
>> This is because any exceptions thrown from the constructor are effectively
>> ignored, and the launcher will continue with the next variant. This seems
>> wrong - the exception should be printed for the user, like:
>>
>> $ java --enable-preview -classpath /tmp/ MainClass
>> Exception in thread "main" java.lang.RuntimeException
>> at MainClass.(MainClass.java:3)
>>
>>
>> This patch proposes to do that by not consuming the exceptions thrown from
>> the constructor, and stop the propagation to the next variant.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
Thanks for the updated version, much easier to read/maintainer.
Minor nit is that you probably should be a space after each "//" to be
consistent with the existing code.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18753#pullrequestreview-2000571601
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
> Consider code like:
>
> public class MainClass {
> public MainClass() {
> System.out.println("Constructor called!");
> }
> public static void main() {
> System.out.println("main called!");
> }
> }
>
> and compile and run it, with preview enabled, like:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> Constructor called!
> main called!
>
>
> That is wrong, as the `main` method is static, and there is no need to create
> a new instance of the class.
>
> The reason is that as launcher attempts to invoke the main method, it goes in
> the following order: 1) static-main-with-args; 2) instance-main-with-args; 3)
> static-main-without-args; 4) instance-main-without-args. But, for the
> instance variants, the code first creates a new instance of the given class,
> and only then attempts to lookup the `main` method, and will pass to the next
> option when the `main` method lookup fails. So, when invoking
> static-main-without-args, the current class instance may be created for
> instance-main-with-args, which will then fail due to the missing
> `main(String[])` method.
>
> The proposed solution to this problem is to simply first do a lookup for the
> `main` method (skipping to the next variant when the given main method does
> not exist, without instantiating the current class).
>
> There is also a relatively closely related problem: what happens when the
> constructor throws an exception?
>
> public class MainClass {
> public MainClass() {
> if (true) throw new RuntimeException();
> }
> public void main() {
> System.out.println("main called!");
> }
> }
>
>
> when compiled an run, this produces no output whatsoever:
>
> $ javac /tmp/MainClass.java
> $ java --enable-preview -classpath /tmp MainClass
> $
>
>
> This is because any exceptions thrown from the constructor are effectively
> ignored, and the launcher will continue with the next variant. This seems
> wrong - the exception should be printed for the user, like:
>
> $ java --enable-preview -classpath /tmp/ MainClass
> Exception in thread "main" java.lang.RuntimeException
> at MainClass.(MainClass.java:3)
>
>
> This patch proposes to do that by not consuming the exceptions thrown from
> the constructor, and stop the propagation to the next variant.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Reflecting review feedback.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18753/files
- new: https://git.openjdk.org/jdk/pull/18753/files/3ad521ae..2022aa5a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18753=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18753=00-01
Stats: 22 lines in 1 file changed: 10 ins; 10 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/18753.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18753/head:pull/18753
PR: https://git.openjdk.org/jdk/pull/18753
Re: RFR: 8329420: Java 22 (and 23) launcher calls default constructor although main() is static [v2]
On Sun, 14 Apr 2024 06:51:37 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback. > > src/java.base/share/native/libjli/java.c line 434: > >> 432: CHECK_EXCEPTION_FAIL(); >> 433: jobject mainObject = (*env)->NewObject(env, mainClass, constructor); >> 434: CHECK_EXCEPTION_NULL_PASS(mainObject); > > There will be a pending exception if NewObject returns NULL. I think it would > be a lot easier to read if this were to just check if mainObject is NULL and > return 1. Same comment for the other usage in invokeInstanceMainWithoutArgs. > That would avoid CHECK_EXCEPTION_NULL_PASS which is very confusing to see at > the use-sites. Thanks! I've done that here: https://github.com/openjdk/jdk/pull/18753/commits/2022aa5a0d21f930d9b49d1bb0b91ecf7e60ead2 - PR Review Comment: https://git.openjdk.org/jdk/pull/18753#discussion_r1565302792
