Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman > @SoniaZaldana CI testing of this PR along with latest master commits passed > without any failures. So you can now integrate this. Thanks @jaikiran! - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2149833060
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman @SoniaZaldana CI testing of this PR along with latest master commits passed without any failures. So you can now integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2148719775
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman > Hello Sonia @SoniaZaldana, so based on Alan's latest inputs, the next thing > to do on this PR is to refresh the PR to merge it with the latest master > branch changes and then run tier1, tier2 and tier3 tests to make sure they > continue to pass and have this integrated soon. Curious, why tier 1 to 3 specifically? Is there anything specific in tier 3 you want to have tested? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147514185
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 4 Jun 2024 13:17:11 GMT, Thomas Stuefe wrote: > Curious, why tier 1 to 3 specifically? Is there anything specific in tier 3 > you want to have tested? I think just prudent to run more tests when touching the launcher as it has options that aren't tested much in tier1. Shouldn't be an issue here but thinking of the splash screen tests (jdk_desktop test group), and tests for Java agents. Also jlink and jpackage can be useful when testing changes to the launcher. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147552797
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 4 Jun 2024 12:59:54 GMT, Jaikiran Pai wrote: > I'll file follow up issue(s) and also trigger CI testing of this PR. Thanks, the regressions fixed here are important to fix. It's unfortunate there the original changes weren't changes weren't caught by tests. There is a good test coverage added here but I think we'll need to see if there are other testing gaps. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147492466
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman Hello Sonia @SoniaZaldana, so based on Alan's latest inputs, the next thing to do on this PR is to refresh the PR to merge it with the latest master branch changes and then run tier1, tier2 and tier3 tests to make sure they continue to pass and have this integrated soon. I'll file follow up issue(s) and also trigger CI testing of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147476871
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman Okay. The follow-on issue can remove CHECK_EXCEPTION_NULL_FAIL introduced by this PR or replace it with something simple that just checks if the ID is null. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147452633
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Decreasing diff size addressing unnecessary changes Hi all, I think there's some consensus that we need some follow up cleanup issues for the JNI spec, renaming constants, fixing return codes, etc. Seeing how that grows the scope of the issue quite a bit, I'd like to push this patch and track the other issues brought up separately. If there are no objections about the current state, I'd like to integrate some time next week. Let me know your thoughts. cc: @jaikiran, @AlanBateman - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2142383991
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Decreasing diff size addressing unnecessary changes Hello Sonia, I happened to be working on something else and ended up hitting an issue where the java launch was no longer throwing the exception from the main application and realized that this PR hasn't been integrated. I see Alan has suggested a change but I'm unsure if you plan to implement that in this PR or if that's something that's suggested for a subsequent cleanup. I think it would be good to have the review suggestions resolved soon and have this integrated. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2141428133
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Tue, 14 May 2024 07:14:09 GMT, Thomas Stuefe wrote: > but it does not state explicitly that an exception is thrown on every error, > or whether there are cases where the API can return NULL but not throw an > exception, or vice versa. > > So, I'd check for both. Or, if we think that both should not happen or happen > together, assert that. (we can use the standard C assert in the JDK > libraries, no?) This same issue comes up periodically and maybe the JNI spec needs improvement to make it clearer that returning NULL means there is a pending exception/error. JNI would be very broken if it were to return a jclass or ID and also raise an exception, or return NULL rather than a jclass or ID when it succeeds. As you say, it's not wrong to check both, just a bit strange as they can be just checks for NULL. At some point I think we need to do a round of cleanup here as these macros can mask other issues. It's not wrong to first check for a first exception, just a bit strange and not immediately clear what issues they may be masking. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2109495552
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Mon, 13 May 2024 18:01:25 GMT, Sonia Zaldana Calles wrote: > > This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI > > functions GetStaticMethodID, GetMethodID and NewObject return NULL with a > > pending exception when they fail. So I would expect > > CHECK_EXCEPTION_NULL_FAIL to just check if obj is NULL rather check for an > > exception first. It's not wrong to check for an exception, just curious > > when looking at this macro. > > Hi @AlanBateman, thanks for taking a look. That's a good point - would it be > worthwhile to delete the exception check in this case? Well, its not wrong, and arguably more defensive. Doc for GetStaticMethodID states: RETURNS: Returns a method ID, or NULL if the operation fails. THROWS: NoSuchMethodError: if the specified static method cannot be found. ExceptionInInitializerError: if the class initializer fails due to an exception. OutOfMemoryError: if the system runs out of memory. but it does not state explicitly that an exception is thrown on every error, or whether there are cases where the API can return NULL but not throw an exception, or vice versa. So, I'd check for both. Or, if we think that both should not happen or happen together, assert that. (we can use the standard C assert in the JDK libraries, no?) - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2109450146
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Sun, 12 May 2024 18:52:30 GMT, Alan Bateman wrote: > This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI > functions GetStaticMethodID, GetMethodID and NewObject return NULL with a > pending exception when they fail. So I would expect CHECK_EXCEPTION_NULL_FAIL > to just check if obj is NULL rather check for an exception first. It's not > wrong to check for an exception, just curious when looking at this macro. Hi @AlanBateman, thanks for taking a look. That's a good point - would it be worthwhile to delete the exception check in this case? - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2108464274
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Decreasing diff size addressing unnecessary changes This mostly looks good. I'm just puzzled CHECK_EXCEPTION_NULL_FAIL. The JNI functions GetStaticMethodID, GetMethodID and NewObject return NULL with a pending exception when they fail. So I would expect CHECK_EXCEPTION_NULL_FAIL to just check if obj is NULL rather check for an exception first. It's not wrong to check for an exception, just curious when looking at this macro. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2106343194
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles wrote: >> Hi folks, >> >> This PR aims to fix >> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). >> >> I think the regression got introduced in >> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). >> >> In the issue linked above, >> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) >> got removed to simplify launcher code. >> >> Previously, we used ```getMainType``` to do the appropriate main method >> invocation in ```JavaMain```. However, we currently attempt to do all types >> of main method invocations at the same time >> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). >> >> >> Note how all of these invocations clear the exception reported with >> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). >> >> >> Therefore, if a legitimate exception comes up during one of these >> invocations, it does not get reported. >> >> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking >> forward to your suggestions. >> >> Cheers, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with one > additional commit since the last revision: > > Decreasing diff size addressing unnecessary changes My remarks have all been addressed. Thank you, @SoniaZaldana. From my side this is good to go. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18786#pullrequestreview-2049599835
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
> Hi folks, > > This PR aims to fix > [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). > > I think the regression got introduced in > [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). > > In the issue linked above, > [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226) > got removed to simplify launcher code. > > Previously, we used ```getMainType``` to do the appropriate main method > invocation in ```JavaMain```. However, we currently attempt to do all types > of main method invocations at the same time > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623). > > > Note how all of these invocations clear the exception reported with > [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390). > > > Therefore, if a legitimate exception comes up during one of these > invocations, it does not get reported. > > I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking > forward to your suggestions. > > Cheers, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision: Decreasing diff size addressing unnecessary changes - Changes: - all: https://git.openjdk.org/jdk/pull/18786/files - new: https://git.openjdk.org/jdk/pull/18786/files/9b67fd86..1a0c76f2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18786&range=08-09 Stats: 9 lines in 1 file changed: 4 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18786.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18786/head:pull/18786 PR: https://git.openjdk.org/jdk/pull/18786
