Re: RFR: 8341135: Incorrect format string after JDK-8339475
On Tue, 1 Oct 2024 12:40:57 GMT, Matthias Baesken wrote:
>> src/java.base/macosx/native/libjli/java_md_macosx.m line 315:
>>
>>> 313: rc = pthread_create(&main_thr, NULL, &apple_main, &args);
>>> 314: if (rc != 0) {
>>> 315: JLI_ReportErrorMessageSys("Could not create main thread,
>>> return code:%d\n", rc);
>>
>> Shouldn't there be a space in between, `return code: %d`?
>
> I can live with both, not sure what is preferred. In the corresponding
> fprintf output of 8339475 there is no space.
I think a space would be nicer. 😄
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21278#discussion_r1782726535
Re: RFR: 8341135: Incorrect format string after JDK-8339475
On Tue, 1 Oct 2024 07:22:46 GMT, Matthias Baesken wrote: > This fixes a format string issue. > > In the error report it was reported : 'The compiler should be able to catch > this if JLI_ReportErrorMessageSys was declared with something like > __attribute__ ((format (printf, 1, 2))).' > Should we maybe adjust JLI_ReportErrorMessageSys , JLI_ReportErrorMessage > and related functions/methods using a format string so that such errors can > be found already at build time ? Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21278#pullrequestreview-2339496280
Re: RFR: 8339364: AIX build fails: various unused variable and function warnings
On Mon, 2 Sep 2024 11:43:20 GMT, Matthias Baesken wrote: > We get a couple of warnings as errors on AIX because of unused variables or > functions , for example : > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:665:10: > error: unused variable 'exePath' [-Werror,-Wunused-variable] > char exePath[PATH_MAX]; > ^ > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:668:9: > error: unused variable 'ret' [-Werror,-Wunused-variable] > int ret; > ^ > /priv/jenkins/client-home/workspace/openjdk-jdk-dev-aix_ppc64-opt/jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:664:10: > error: unused variable 'fn' [-Werror,-Wunused-variable] > char fn[32]; > ^ > > This seems to be related to the recent make changes > 8339156: Use more fine-granular clang unused warnings > https://bugs.openjdk.org/browse/JDK-8339156 > (we use clang too on AIX) Looks good and seems like the warning helped to clean out coding in a few places. src/java.desktop/unix/native/common/awt/X11Color.c line 1236: > 1234: for (int i = 0; i < num_colors; i++) > 1235: alloc_col (awt_display, awtData->awt_cmap, red (rgbColors [i]), > 1236: green (rgbColors [i]), blue (rgbColors [i]), -1, Seems like indentation here could be improved a bit. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20812#pullrequestreview-2275486151 PR Review Comment: https://git.openjdk.org/jdk/pull/20812#discussion_r1740824222
[jdk23] Integrated: 8222884: ConcurrentClassDescLookup.java times out intermittently
On Mon, 24 Jun 2024 09:40:14 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8222884](https://bugs.openjdk.org/browse/JDK-8222884), commit > [bd046d9b](https://github.com/openjdk/jdk/commit/bd046d9b9e79e4eea89c72af358961ef6e98e660) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jaikiran Pai on 12 Jun 2024 and > was reviewed by Roger Riggs and Matthias Baesken. > > Thanks! This pull request has now been integrated. Changeset: 08c7c383 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/08c7c38342809c60f5fdea70717362a72b00f6e9 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod 8222884: ConcurrentClassDescLookup.java times out intermittently Reviewed-by: mdoerr Backport-of: bd046d9b9e79e4eea89c72af358961ef6e98e660 - PR: https://git.openjdk.org/jdk/pull/19853
[jdk23] RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently
Hi all, This pull request contains a backport of [JDK-8222884](https://bugs.openjdk.org/browse/JDK-8222884), commit [bd046d9b](https://github.com/openjdk/jdk/commit/bd046d9b9e79e4eea89c72af358961ef6e98e660) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Jaikiran Pai on 12 Jun 2024 and was reviewed by Roger Riggs and Matthias Baesken. Thanks! - Commit messages: - Backport bd046d9b9e79e4eea89c72af358961ef6e98e660 Changes: https://git.openjdk.org/jdk/pull/19853/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19853&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8222884 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19853.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19853/head:pull/19853 PR: https://git.openjdk.org/jdk/pull/19853
Re: [jdk23] RFR: 8334441: Mark tests in jdk_security_infra group as manual
On Sat, 22 Jun 2024 08:07:54 GMT, SendaoYan wrote: > Hi all, > > This pull request contains a backport of commit > [8e1d2b09](https://github.com/openjdk/jdk/commit/8e1d2b091c9a311d98a0b886a803fb18d4405d8a) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Rajan Halade on 21 Jun 2024 and > was reviewed by Christoph Langer and Sean Mullan. > > Thanks! Thanks for doing the backport. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19841#pullrequestreview-2133855195
Re: RFR: 8334441: Mark tests in jdk_security_infra group as manual
On Thu, 20 Jun 2024 18:35:00 GMT, Rajan Halade wrote: > Updated all the tests that depend on external infrastructure services as > manual. These tests may fail with external reasons, for instance - change in > CA test portal, certificate status updates, or network issues. Looks good, although I don't see why sometimes the directive is @run main/othervm/manual/manual (double occurence of manual). test/jdk/security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java line 30: > 28: * @library /test/lib > 29: * @build jtreg.SkippedException ValidatePathWithURL CAInterop > 30: * @run main/othervm/manual/manual -Djava.security.debug=certpath,ocsp What is the reason why manual is written twice? (Here and in all other places in this file...) - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19814#pullrequestreview-2132623582 PR Review Comment: https://git.openjdk.org/jdk/pull/19814#discussion_r1648943324
[jdk23] Integrated: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Mon, 17 Jun 2024 08:19:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit > [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Christoph Langer on 13 Jun 2024 > and was reviewed by Thomas Stuefe. > > Thanks! This pull request has now been integrated. Changeset: 4e3bfc92 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/4e3bfc926ebdf512c7b9dbddc8caa7b66e2777f7 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected" Reviewed-by: mbaesken Backport-of: f5213671f7b636b32bb93c78e43696a61cd69bae - PR: https://git.openjdk.org/jdk/pull/19742
[jdk23] RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
Hi all, This pull request contains a backport of [JDK-8211847](https://bugs.openjdk.org/browse/JDK-8211847), commit [f5213671](https://github.com/openjdk/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Christoph Langer on 13 Jun 2024 and was reviewed by Thomas Stuefe. Thanks! - Commit messages: - Backport f5213671f7b636b32bb93c78e43696a61cd69bae Changes: https://git.openjdk.org/jdk/pull/19742/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19742&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8211847 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19742.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19742/head:pull/19742 PR: https://git.openjdk.org/jdk/pull/19742
Integrated: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. This pull request has now been integrated. Changeset: f5213671 Author: Christoph Langer URL: https://git.openjdk.org/jdk/commit/f5213671f7b636b32bb93c78e43696a61cd69bae Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected" Reviewed-by: stuefe - PR: https://git.openjdk.org/jdk/pull/19691
Re: RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. Trivial fix of test listing, so - PR Comment: https://git.openjdk.org/jdk/pull/19691#issuecomment-2165639694
Re: RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
On Thu, 13 Jun 2024 09:47:25 GMT, Christoph Langer wrote: > It seems the error is gone meanwhile. So we can reenable the test. Maybe [this one](https://github.com/openjdk/jdk/commit/4d9042043ecade75d50c25574a445e6b8ef43618)? But just guessing... - PR Comment: https://git.openjdk.org/jdk/pull/19691#issuecomment-2165243238
RFR: 8211847: [aix] java/lang/ProcessHandle/InfoTest.java fails: "reported cputime less than expected"
It seems the error is gone meanwhile. So we can reenable the test. - Commit messages: - JDK-8211847 Changes: https://git.openjdk.org/jdk/pull/19691/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19691&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8211847 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19691.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19691/head:pull/19691 PR: https://git.openjdk.org/jdk/pull/19691
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v4]
On Wed, 15 May 2024 11:40:38 GMT, Joachim Kern wrote: >> Since ~ end of March, after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), >> tools/launcher/JliLaunchTest.java fails on AIX. Failure is : >> >> stdout: []; >> stderr: [Error: could not find libjava.so >> Error: Could not find Java SE Runtime Environment. >> ] >> exitValue = 2 >> >> java.lang.RuntimeException: Expected to get exit value of [0], exit value >> is: [2] >> at >> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521) >> at JliLaunchTest.main(JliLaunchTest.java:58) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) >> at java.base/java.lang.Thread.run(Thread.java:1575) >> >> Maybe we need to do further adjustments to >> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / >> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ? >> Or somehow adjust the coding that attempts to find parts of "JRE" >> (libjava.so) ? The libjli.so is gone on AIX after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this >> also the image discovery based on GetApplicationHomeFromDll which uses >> libjli.so . >> >> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. >> There is no other methos available on AIX, because it lacks the $ORIGIN >> feature of the rpath. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes src/java.base/unix/native/libjli/java_md_common.c line 128: > 126: /* > 127: * Retrieves the path to the JRE home by locating libjava.so in > 128: * one of the LIBPATH and then truncating the path to it. Suggestion: * LIBPATH and then truncating the path to it. Another small comment suggestion - PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1601491722
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v3]
On Tue, 7 May 2024 08:08:05 GMT, Joachim Kern wrote: >> Since ~ end of March, after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), >> tools/launcher/JliLaunchTest.java fails on AIX. Failure is : >> >> stdout: []; >> stderr: [Error: could not find libjava.so >> Error: Could not find Java SE Runtime Environment. >> ] >> exitValue = 2 >> >> java.lang.RuntimeException: Expected to get exit value of [0], exit value >> is: [2] >> at >> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521) >> at JliLaunchTest.main(JliLaunchTest.java:58) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333) >> at java.base/java.lang.Thread.run(Thread.java:1575) >> >> Maybe we need to do further adjustments to >> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest / >> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ? >> Or somehow adjust the coding that attempts to find parts of "JRE" >> (libjava.so) ? The libjli.so is gone on AIX after >> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this >> also the image discovery based on GetApplicationHomeFromDll which uses >> libjli.so . >> >> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar. >> There is no other methos available on AIX, because it lacks the $ORIGIN >> feature of the rpath. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19000#pullrequestreview-2057562812
Re: RFR: 8329653: JLILaunchTest fails on AIX after JDK-8329131 [v2]
On Fri, 3 May 2024 15:25:05 GMT, Joachim Kern wrote:
>> Since ~ end of March, after
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131),
>> tools/launcher/JliLaunchTest.java fails on AIX. Failure is :
>>
>> stdout: [];
>> stderr: [Error: could not find libjava.so
>> Error: Could not find Java SE Runtime Environment.
>> ]
>> exitValue = 2
>>
>> java.lang.RuntimeException: Expected to get exit value of [0], exit value
>> is: [2]
>> at
>> jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:521)
>> at JliLaunchTest.main(JliLaunchTest.java:58)
>> at
>> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
>> at
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
>> at java.base/java.lang.Thread.run(Thread.java:1575)
>>
>> Maybe we need to do further adjustments to
>> BUILD_JDK_JTREG_EXECUTABLES_LIBS_exeJliLaunchTest /
>> BUILD_JDK_JTREG_EXECUTABLES_LDFLAGS_exeJliLaunchTest on AIX ?
>> Or somehow adjust the coding that attempts to find parts of "JRE"
>> (libjava.so) ? The libjli.so is gone on AIX after
>> [JDK-8329131](https://bugs.openjdk.org/browse/JDK-8329131), and with this
>> also the image discovery based on GetApplicationHomeFromDll which uses
>> libjli.so .
>>
>> Without libjli.so we have to analyze the LD-LIBRARY_PATH / LIBPATH envvar.
>> There is no other methos available on AIX, because it lacks the $ORIGIN
>> feature of the rpath.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> only for AIX
Minor formatting suggestions
src/java.base/unix/native/libjli/java_md_common.c line 135:
> 133: */
> 134: jboolean
> 135: GetApplicationHomeFromLibpath(char *buf, jint bufsize)
Suggestion:
GetApplicationHomeFromLibpath(char *buf, jint bufsize) {
src/java.base/unix/native/libjli/java_md_common.c line 136:
> 134: jboolean
> 135: GetApplicationHomeFromLibpath(char *buf, jint bufsize)
> 136: {
Suggestion:
src/java.base/unix/native/libjli/java_md_common.c line 139:
> 137: char *env = getenv(LD_LIBRARY_PATH);
> 138: char *tmp;
> 139: char* save_ptr = NULL;
Suggestion:
char *save_ptr = NULL;
-
PR Review: https://git.openjdk.org/jdk/pull/19000#pullrequestreview-2042227569
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591912792
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591912990
PR Review Comment: https://git.openjdk.org/jdk/pull/19000#discussion_r1591911912
Re: RFR: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing [v6]
On Thu, 25 Apr 2024 13:22:01 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove /jre path check Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18699#pullrequestreview-2024359622
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v6]
On Thu, 25 Apr 2024 13:22:01 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove /jre path check Still looks good. We should maybe change the synopsis (title) of the bug to something like "libjli GetApplicationHome cleanups"? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2077289573
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v4]
On Fri, 19 Apr 2024 10:07:22 GMT, Matthias Baesken wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > remove launcher executable path trace output It helped to understand the issue behind https://bugs.openjdk.org/browse/JDK-8329653 - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2070365643
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
On Tue, 16 Apr 2024 10:20:23 GMT, Alan Bateman wrote: >> We have already good JLI tracing capabilities. But GetApplicationHome and >> GetApplicationHomeFromDll lack some tracing and should be enhanced. > > I think this is way too ad hoc and looks like lefts over from a debugging > session. So I don't think it should be integrated without stepping back and > thinking more about what this tracing option is intended for. Generally this looks more consistent now. Let's see what @AlanBateman thinks... 😄 - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2066177891
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing [v3]
On Thu, 18 Apr 2024 06:57:05 GMT, Matthias Baesken wrote:
>> We have already good JLI tracing capabilities. But GetApplicationHome and
>> GetApplicationHomeFromDll lack some tracing and should be enhanced.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove obsolete USE_REGISTRY_LOOKUP usages
src/java.base/unix/native/libjli/java_md_common.c line 85:
> 83: const char *execname = GetExecName();
> 84: if (execname != NULL) {
> 85: JLI_TraceLauncher("Launcher executable path is %s\n", execname);
This trace seems a bit unsymmetric to its Windows counterpart. Maybe it should
be left out here, too, since there is tracing in GetJREPath.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18699#discussion_r1572086726
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
On Mon, 15 Apr 2024 12:47:08 GMT, Matthias Baesken wrote: > > If we expand the tracing then I think it should be consistent with the > > existing tracing. > > What exactly do you see as inconsistent ? Maybe the output of the tracing should look similar to other traces done through `JLI_TraceLauncher`? E.g. not mention method names but just tell what the program is doing... ? - PR Comment: https://git.openjdk.org/jdk/pull/18699#issuecomment-2058621283
Re: RFR: 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing
On Tue, 9 Apr 2024 15:28:08 GMT, Matthias Baesken wrote:
> We have already good JLI tracing capabilities. But GetApplicationHome and
> GetApplicationHomeFromDll lack some tracing and should be enhanced.
To me this looks useful, although maybe the overall JLI tracing could be
revisited.
src/java.base/windows/native/libjli/java_md.c line 326:
> 324: }
> 325:
> 326: JLI_TraceLauncher("GetJREPath - attempt to get JRE location from
> shared lib of the image\n");
Maybe add a trace also in the USE_REGISTRY_LOOKUP section
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18699#pullrequestreview-2003079325
PR Review Comment: https://git.openjdk.org/jdk/pull/18699#discussion_r1567018779
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
Obvious typo that broke the AIX build. Thanks for fixing.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18484#pullrequestreview-1959820693
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 4 Mar 2024 15:02:45 GMT, Aleksei Efimov wrote: >>> As for the test, I had a closer look now and I find it hard to separate >>> testing of [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) from >>> [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). Furthermore, >>> most of the entries test things that hadn't been addressed by any of these >>> two bugs at all. >>> >>> So, [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) is only >>> tested in lines 72, 73, 76 and 77 The original problem of this issue >>> [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) is touched in >>> line 71 and 73. >>> >>> That means, most of the other test invocations test some generic behavior >>> which was never erroneous so far. >> >> Thanks for exploring the possibility of improving tracebility of test >> invocations to reported bugs. >> >>> I could, however, give each line its own test id and annotate the bugs >>> accordingly. Do you think that makes sense? >> >> It does make sense, but I'm not sure how such annotations will look like and >> if it will be easy to use them for debugging failures. I will leave the >> final decision to you here. Your last message with linkage of test >> invocations to bug id is already a good information to have. >> >>> I drafted a CSR. @AlekseiEfimov, would be nice if you could review it. >> >> Thanks for drafting a CSR. I will review it in comming days. > >> Thanks for exploring the possibility of improving tracebility of test >> invocations to reported bugs. >> >> > > > I've given this test change a second thought, maybe you can try to separate > the test into two separate test classes? One possibility to avoid duplicating > code and have a separate test for each bug is to introduce a base test class > that will contain the common functionality for > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) and > [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) tests. Thanks @AlekseiEfimov and @dfuch for the reviews. Submitting this now. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-2018412400
Integrated: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
On Fri, 9 Feb 2024 21:29:28 GMT, Christoph Langer wrote: > During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? This pull request has now been integrated. Changeset: 907e30ff Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/907e30ff00abd6cd4935987810d282f46ec07704 Stats: 245 lines in 3 files changed: 130 ins; 35 del; 80 mod 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket Reviewed-by: dfuchs, aefimov - PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v9]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Review suggestions Aleksei - Merge branch 'master' into JDK-8325579 - Update module-info text - Merge branch 'master' into JDK-8325579 - Indentation - Merge branch 'master' into JDK-8325579 - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - ... and 4 more: https://git.openjdk.org/jdk/compare/7b45211e...8fdc039c - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/10271159..8fdc039c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=07-08 Stats: 292653 lines in 398 files changed: 5187 ins; 5644 del; 281822 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
On Tue, 19 Mar 2024 16:09:18 GMT, Aleksei Efimov wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Update module-info text
>> - Merge branch 'master' into JDK-8325579
>> - Indentation
>> - Merge branch 'master' into JDK-8325579
>> - Review feedback
>> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing
>> - Merge branch 'master' into JDK-8325579
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/ffdffc5b...10271159
>
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 164:
>
>> 162: if
>> (CustomSocketFactory.customSocket.closeMethodCalledCount() <= 0) {
>> 163: System.err.println("Custom Socket was not
>> closed.");
>> 164: System.exit(-1);
>
> Can we update test not to use `System.exit` and replace it with throwing `new
> RuntimeException`, something like:
> Suggestion:
>
> throw new RuntimeException("Custom Socket was not
> closed");
Done.
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 177:
>
>> 175: ctx.close();
>> 176: if (!checkSocketClosed(sock)) {
>> 177: System.exit(-1);
>
> Can be replaced with:
> Suggestion:
>
> throw new RuntimeException("Socket isn't closed");
I simplified this, removing checkSocketClosed() method
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 184:
>
>> 182: e.printStackTrace();
>> 183: System.exit(-1);
>> 184: }
>
> For simplification and `System.exit` removal purposes this catch block can be
> removed with addition of `throws Exception` clause to the `main` method.
> Suggestion:
>
> }
I chose to throw a new RuntimeException(e)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532883964
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532885041
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532884603
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
On Tue, 19 Mar 2024 16:01:34 GMT, Aleksei Efimov wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 12 additional
>> commits since the last revision:
>>
>> - Update module-info text
>> - Merge branch 'master' into JDK-8325579
>> - Indentation
>> - Merge branch 'master' into JDK-8325579
>> - Review feedback
>> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing
>> - Merge branch 'master' into JDK-8325579
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/b172eb4c...10271159
>
> src/java.naming/share/classes/module-info.java line 42:
>
>> 40: * The value of this environment property specifies the fully
>> 41: * qualified class name of the socket factory used by the LDAP
>> provider.
>> 42: * This class must implement the javax.net.SocketFactory
>> abstract class
>
> You could add a link to the `SocketFactory` class here:
> Suggestion:
>
> * This class must implement the {@link javax.net.SocketFactory}
> abstract class
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1532877183
Integrated: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
On Wed, 13 Mar 2024 07:53:30 GMT, Christoph Langer wrote: > 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` This pull request has now been integrated. Changeset: 128e60a2 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/128e60a29f1bd1e1fbe165ac382107070858ecc6 Stats: 650 lines in 1 file changed: 343 ins; 269 del; 38 mod 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 Reviewed-by: rgiulietti - PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
On Wed, 13 Mar 2024 14:23:01 GMT, Raffaello Giulietti wrote: >> Christoph Langer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Generate large strings in parameter generator methods > > I was thinking more something like > > var tenMillionBlanks = tenMillionBlanks(); > > and similarly for `tenMillionsZeros`, thus maintaining the `private static` > (but not `final` ;-) ) methods as in your previous commit. > But if you are happy with the last commit, it's OK as well. > > Can you please add the bug id to `@bug` and correct the typo, as suggested > [here](https://github.com/openjdk/jdk/pull/18264#issuecomment-1994382507)? > > Otherwise looks fine. Thanks, @rgiulietti for reviewing this. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1999033956
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Thu, 7 Mar 2024 17:37:59 GMT, Christoph Langer wrote: > > > Thanks for exploring the possibility of improving tracebility of test > > > invocations to reported bugs. > > > > > > > > > > I've given this test change a second thought, maybe you can try to separate > > the test into two separate test classes? One possibility to avoid > > duplicating code and have a separate test for each bug is to introduce a > > base test class that will contain the common functionality for > > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) and > > [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) tests. > > I will have a look. I stared at the test once more but it still doesn't make sense to me to split it into multiple tests because the purposes of each test run intertwine, as stated [above](https://github.com/openjdk/jdk/pull/17797#issuecomment-1959271452). The only thing that would really make sense is to rename from LdapSSLHandshakeFailureTest to LdapSSLHandshakeTest since it in fact tests various variations of the SSL handshaking. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1999031201
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v7]
On Thu, 7 Mar 2024 17:38:24 GMT, Christoph Langer wrote: >> src/java.naming/share/classes/module-info.java line 42: >> >>> 40: * The value of this environment property specifies the >>> fully >>> 41: * qualified class name of the socket factory used by the LDAP >>> provider. >>> 42: * This class must implement the javax.net.SocketFactory >>> abstract class. >> >> We need to mention here that `getDefault` method needs to be implemented by >> the provided socket factory class. >> >> Something like: `and provide an implementation of the static "getDefault()" >> method that returns an instance of the socket factory.` >> The CSR needs to be updated too. > > Good point. I will amend that in both, source code and CSR. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1525530951
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v8]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Update module-info text - Merge branch 'master' into JDK-8325579 - Indentation - Merge branch 'master' into JDK-8325579 - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - ... and 2 more: https://git.openjdk.org/jdk/compare/584e58bb...10271159 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/97299970..10271159 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=06-07 Stats: 97262 lines in 1251 files changed: 15655 ins; 78153 del; 3454 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v3]
> 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Review suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/18264/files - new: https://git.openjdk.org/jdk/pull/18264/files/be84e5e8..ca3ec0d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
On Wed, 13 Mar 2024 14:23:01 GMT, Raffaello Giulietti wrote: > Can you please add the bug id to `@bug` and correct the typo, as suggested > [here](https://github.com/openjdk/jdk/pull/18264#issuecomment-1994382507)? Done. I kept the tenMillion... handling. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1995807615
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718 [v2]
> 4f336085d1098e7fba7b58f0a73c028179a2a13d > ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few > cases to test java/util/Formatter/Padding.java with huge Strings as > arguments. Since all possible argument combinations for the test are stored > in one array, nothing can be garbage collected while the test is running and > the heap requirement is blown up. > > In one of our test pipelines we run tier1 tests with VMs that default to 384M > of heap and this is not sufficient any longer. > > I'm improving this by splitting the one large @ParameterizedTest into > multiple ones. With that, I could run the test successfully in a test VM with > 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm > -Xmx96m Padding` Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Generate large strings in parameter generator methods - Changes: - all: https://git.openjdk.org/jdk/pull/18264/files - new: https://git.openjdk.org/jdk/pull/18264/files/8e3e9509..be84e5e8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=00-01 Stats: 53 lines in 1 file changed: 9 ins; 8 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
On Wed, 13 Mar 2024 09:42:34 GMT, Raffaello Giulietti wrote: > What about factoring out the 4 invocations of `tenMillionBlanks()` in each > source method in a local var? OK, I inlined the generation of the ten million character strings into the parameter generator methods. I looked a bit at the test runtime and it seems like it doesn't make a lot of difference in a test JVM with larger heap but for smaller test VMs it seems to improve things. - PR Comment: https://git.openjdk.org/jdk/pull/18264#issuecomment-1994497012
RFR: 8328037: Test java/util/Formatter/Padding.java has unnecessary high heap requirement after JDK-8326718
4f336085d1098e7fba7b58f0a73c028179a2a13d ([JDK-8326718](https://bugs.openjdk.org/browse/JDK-8326718)) added a few cases to test java/util/Formatter/Padding.java with huge Strings as arguments. Since all possible argument combinations for the test are stored in one array, nothing can be garbage collected while the test is running and the heap requirement is blown up. In one of our test pipelines we run tier1 tests with VMs that default to 384M of heap and this is not sufficient any longer. I'm improving this by splitting the one large @ParameterizedTest into multiple ones. With that, I could run the test successfully in a test VM with 96M of heap, e.g. by modifying `@run junit Padding` to `@run junit/othervm -Xmx96m Padding` - Commit messages: - JDK-8328037 Changes: https://git.openjdk.org/jdk/pull/18264/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18264&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328037 Stats: 473 lines in 1 file changed: 168 ins; 95 del; 210 mod Patch: https://git.openjdk.org/jdk/pull/18264.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18264/head:pull/18264 PR: https://git.openjdk.org/jdk/pull/18264
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v7]
On Wed, 6 Mar 2024 14:42:09 GMT, Aleksei Efimov wrote: >> Christoph Langer has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - Indentation >> - Merge branch 'master' into JDK-8325579 >> - Review feedback >> - Rename back to LdapSSLHandshakeFailureTest to ease reviewing >> - Merge branch 'master' into JDK-8325579 >> - Typo >> - Merge branch 'master' into JDK-8325579 >> - Rename test and refine comment >> - Enhance test >> - JDK-8325579 > > src/java.naming/share/classes/module-info.java line 42: > >> 40: * The value of this environment property specifies the fully >> 41: * qualified class name of the socket factory used by the LDAP >> provider. >> 42: * This class must implement the javax.net.SocketFactory >> abstract class. > > We need to mention here that `getDefault` method needs to be implemented by > the provided socket factory class. > > Something like: `and provide an implementation of the static "getDefault()" > method that returns an instance of the socket factory.` > The CSR needs to be updated too. Good point. I will amend that in both, source code and CSR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1516570214
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 4 Mar 2024 15:02:45 GMT, Aleksei Efimov wrote: > > Thanks for exploring the possibility of improving tracebility of test > > invocations to reported bugs. > > > > > I've given this test change a second thought, maybe you can try to separate > the test into two separate test classes? One possibility to avoid duplicating > code and have a separate test for each bug is to introduce a base test class > that will contain the common functionality for > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) and > [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) tests. I will have a look. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1984087841
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v4]
On Thu, 7 Mar 2024 08:16:26 GMT, Matthias Baesken wrote: >> We define the RESTARTABLE macro again and again at a lot of places in the >> JDK native codebase. This could be centralized to avoid repeating it again >> and again ! > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust COPYRIGHT year info Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1921955570
Re: RFR: JDK-8327444: simplify RESTARTABLE macro usage in JDK codebase [v3]
On Wed, 6 Mar 2024 16:30:23 GMT, Matthias Baesken wrote:
>> We define the RESTARTABLE macro again and again at a lot of places in the
>> JDK native codebase. This could be centralized to avoid repeating it again
>> and again !
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Introduce windows jni_util_md.h
Looks good to me modulo a few license year things...
src/java.base/macosx/native/libnio/ch/KQueue.c line 2:
> 1: /*
> 2: * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights
> reserved.
Here you only change the license year?
src/java.base/share/native/libjava/jni_util.h line 30:
> 28:
> 29: #include "jni.h"
> 30: #include "jni_util_md.h"
This file needs copyright year update
src/java.base/unix/native/libjava/childproc.h line 75:
> 73: #define FAIL_FILENO (STDERR_FILENO + 1)
> 74:
> 75: /* TODO: Refactor. */
Copyright year update missing.
src/java.base/unix/native/libnio/ch/nio_util.h line 34:
> 32: #include
> 33:
> 34: #define RESTARTABLE(_cmd, _result) do { \
Copyright year update
src/java.base/unix/native/libnio/fs/UnixFileSystem.c line 38:
> 36: #include "sun_nio_fs_UnixFileSystem.h"
> 37:
> 38: #define RESTARTABLE(_cmd, _result) do { \
Copyright year update
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18132#pullrequestreview-1920355323
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514867262
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514873505
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514876266
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514878580
PR Review Comment: https://git.openjdk.org/jdk/pull/18132#discussion_r1514878995
Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer wrote: > The new test JmodExcludedFiles.java checks that no debug symbol files are > contained in the jmod files. It does not take into account that with > configure option --with-external-symbols-in-bundles=public we want to have > stripped pdb files, also in jmods, to get native callstacks with line number. > > This modifies the test and checks if equivalent *stripped.pdb files exist > when .pdb files are encountered inside jmods. In that case one can assume > that --with-external-symbols-in-bundles=public was chosen as configure option. Thanks for the review, Matthias. - PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1976989827
Integrated: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer wrote: > The new test JmodExcludedFiles.java checks that no debug symbol files are > contained in the jmod files. It does not take into account that with > configure option --with-external-symbols-in-bundles=public we want to have > stripped pdb files, also in jmods, to get native callstacks with line number. > > This modifies the test and checks if equivalent *stripped.pdb files exist > when .pdb files are encountered inside jmods. In that case one can assume > that --with-external-symbols-in-bundles=public was chosen as configure option. This pull request has now been integrated. Changeset: 43c6f0b5 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/43c6f0b5880899b797fab2f851bd35be1c342439 Stats: 48 lines in 1 file changed: 30 ins; 5 del; 13 mod 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used Reviewed-by: mbaesken - PR: https://git.openjdk.org/jdk/pull/17990
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v7]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Indentation - Merge branch 'master' into JDK-8325579 - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - Enhance test - JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/ec6579d2..97299970 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=05-06 Stats: 17321 lines in 1249 files changed: 9833 ins; 3144 del; 4344 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Wed, 28 Feb 2024 12:32:17 GMT, Matthias Baesken wrote: > Looks okay to me, but could we print here `RuntimeException(jmodFile + " is > expected not to include native debug symbols` not only the jmod but also the > unwanted file(s) ? This information is already printed in isNativeDebugSymbol. So in case of failure, you'll find the culprit in the test output. - PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1972000461
Re: RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
On Fri, 23 Feb 2024 19:18:46 GMT, Christoph Langer wrote: > The new test JmodExcludedFiles.java checks that no debug symbol files are > contained in the jmod files. It does not take into account that with > configure option --with-external-symbols-in-bundles=public we want to have > stripped pdb files, also in jmods, to get native callstacks with line number. > > This modifies the test and checks if equivalent *stripped.pdb files exist > when .pdb files are encountered inside jmods. In that case one can assume > that --with-external-symbols-in-bundles=public was chosen as configure option. @mlchung, mind to have a look here? Thanks 😄 - PR Comment: https://git.openjdk.org/jdk/pull/17990#issuecomment-1968346603
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v4]
On Tue, 27 Feb 2024 16:48:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust ms Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17952#pullrequestreview-1905420462
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Thu, 22 Feb 2024 16:01:19 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > assertEquals adjust output Then maybe it should be > ```diff > +fail((msg == null ? "assertEquals" : msg) + " expected: " + lhs > + " but was: " + rhs); > ``` ? - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1963086026
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Sat, 24 Feb 2024 11:45:46 GMT, Jaikiran Pai wrote: > This is similar to what other test libraries usually report for such failures. But in the case of a non-empty `msg` you would not see the actual values any more which I think could be helpful in a lot of cases... - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1962412001
RFR: 8326591: New test JmodExcludedFiles.java fails on Windows when --with-external-symbols-in-bundles=public is used
The new test JmodExcludedFiles.java checks that no debug symbol files are contained in the jmod files. It does not take into account that with configure option --with-external-symbols-in-bundles=public we want to have stripped pdb files, also in jmods, to get native callstacks with line number. This modifies the test and checks if equivalent *stripped.pdb files exist when .pdb files are encountered inside jmods. In that case one can assume that --with-external-symbols-in-bundles=public was chosen as configure option. - Commit messages: - JDK-8326591 Changes: https://git.openjdk.org/jdk/pull/17990/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17990&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326591 Stats: 48 lines in 1 file changed: 30 ins; 5 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/17990.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17990/head:pull/17990 PR: https://git.openjdk.org/jdk/pull/17990
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v3]
On Thu, 22 Feb 2024 16:01:19 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > assertEquals adjust output Looks good from my end. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17952#pullrequestreview-1897634635
Re: RFR: JDK-8326389: [test] improve assertEquals failure output [v2]
On Thu, 22 Feb 2024 14:57:05 GMT, Matthias Baesken wrote: >> Currently assertEquals has in the failure case sometimes confusing output >> like : >> >> java.lang.RuntimeException: VM output should contain exactly one RTM locking >> statistics entry for method >> compiler.rtm.locking.TestRTMTotalCountIncrRate$Test:🔒 expected 0 to equal 1 >>at jdk.test.lib.Asserts.fail(Asserts.java:634) >>at jdk.test.lib.Asserts.assertEquals(Asserts.java:205) >> >> (I don't think we really expected that for some reason 0 equals 1) >> This should be improved. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust COPYRIGHT year info I think it is a good idea to improve this. I was irritated by that output more than once. Maybe a better message would be ... _"..." is not equal to "..."_ ? - PR Comment: https://git.openjdk.org/jdk/pull/17952#issuecomment-1959680638
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Wed, 21 Feb 2024 18:26:18 GMT, Aleksei Efimov wrote: >>> Currently, it is hard to distinguish what part of the test responsible for >>> [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) testing, and >>> which part is for >>> [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). I would prefer >>> to add a new test for the current fix instead: that could be done as >>> additional test mode, OR even better to add a completely new test. >> >> Hm, I think the test was overpurposed already. Creating another test file >> with duplicating code does not sound too good IMHO. Maybe it is acceptable >> without the renaming? >> >> Another question: Do we need a CSR/Release note as there is some behavioral >> change involved (although it should always have been like with this change)? > >> Hm, I think the test was overpurposed already. > > This test was added by JDK-8314063 fix, and I do not think it was change > after that. > >> Creating another test file with duplicating code does not sound too good >> IMHO. Maybe it is acceptable without the renaming? > > I think it is acceptable. Currently, it is hard to separate the test cases > for `a)` the original > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) fix (the opened > socket is not closed properly when connection timeout occurs) and `b)` the > current fix [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) > (unconnected sockets are not supported by SocketFactory). It would be great > to have this distinction in the modified test. I drafted a CSR. @AlekseiEfimov, would be nice if you could review it. As for the test, I had a closer look now and I find it hard to separate testing of [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) from [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). Furthermore, most of the entries test things that hadn't been addressed by any of these two bugs at all. So, [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) is only tested in lines 72, 73, 76 and 77 The original problem of this issue [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579) is touched in line 71 and 73. That means, most of the other test invocations test some generic behavior which was never erroneous so far. I could, however, give each line its own test id and annotate the bugs accordingly. Do you think that makes sense? - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1959271452
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 16:46:18 GMT, Aleksei Efimov wrote: > Currently, it is hard to distinguish what part of the test responsible for > [JDK-8314063](https://bugs.openjdk.org/browse/JDK-8314063) testing, and which > part is for [JDK-8325579](https://bugs.openjdk.org/browse/JDK-8325579). I > would prefer to add a new test for the current fix instead: that could be > done as additional test mode, OR even better to add a completely new test. Hm, I think the test was overpurposed already. Creating another test file with duplicating code does not sound too good IMHO. Maybe it is acceptable without the renaming? Another question: Do we need a CSR/Release note as there is some behavioral change involved (although it should always have been like with this change)? - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1953911292
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 16:57:23 GMT, Aleksei Efimov wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - Enhance test
>> - JDK-8325579
>
> src/java.naming/share/classes/module-info.java line 51:
>
>> 49: * network times out.
>> 50: * If a custom socket factory is provided via property
>> 51: * {@code java.naming.ldap.factory.socket} and unconnected
>> sockets
>
> Suggestion:
>
> * If a custom socket factory is provided via {@code
> java.naming.ldap.factory.socket} environment
> *property and unconnected sockets
Addressed.
> src/java.naming/share/classes/module-info.java line 51:
>
>> 49: * network times out.
>> 50: * If a custom socket factory is provided via property
>> 51: * {@code java.naming.ldap.factory.socket} and unconnected
>> sockets
>
> Since we're referencing socket factory env property, maybe we can also
> document it here. Something like:
>
> {@code java.naming.ldap.factory.socket}:
> The value of this environment property specifies the fully
> qualified class name of the socket factory used by the LDAP provider.
> This class must implement the javax.net.SocketFactory abstract class.
> By default the environment property is not set.
>
Added.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495580384
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495580620
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Mon, 19 Feb 2024 13:17:48 GMT, Goetz Lindenmaier wrote:
>> Christoph Langer has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Typo
>> - Merge branch 'master' into JDK-8325579
>> - Rename test and refine comment
>> - Enhance test
>> - JDK-8325579
>
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 224:
>
>> 222:
>> 223: public CustomSocket(String s, int timeout) throws IOException {
>> 224: super(s, timeout);
>
> The argument should be called "port" not timeout.
Correct.
> test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 246:
>
>> 244:
>> 245: @Override
>> 246: public Socket createSocket(String s, int timeout) throws
>> IOException {
>
> The argument should be "int port" instead of timeout.
Right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579995
PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579746
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
On Fri, 16 Feb 2024 16:39:33 GMT, Daniel Fuchs wrote: >> Christoph Langer has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Typo >> - Merge branch 'master' into JDK-8325579 >> - Rename test and refine comment >> - Enhance test >> - JDK-8325579 > > test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeTest.java line 62: > >> 60: * whether the socket is closed after closing the Context. >> 61: * >> 62: * @run main/othervm --add-opens java.naming/javax.naming=ALL-UNNAMED >> --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED > > sigh... git handles the renaming of this test file as a deleted file + a new > file which makes it hard to review the changes. > > The --add-opens directive are very strange here. I suggest removing them and > replacing them with a single `@modules` directive: > > > @modules java.naming/javax.naming:+open > java.naming/com.sun.jndi.ldap:+open Good suggestion. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/17797#discussion_r1495579467
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v6]
On Tue, 20 Feb 2024 09:45:22 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Review feedback > - Rename back to LdapSSLHandshakeFailureTest to ease reviewing > - Merge branch 'master' into JDK-8325579 > - Typo > - Merge branch 'master' into JDK-8325579 > - Rename test and refine comment > - Enhance test > - JDK-8325579 I made some updates and addressed the review comments. I reverted the renaming of the test for the sake of reviewing. While I still think LdapSSLHandshakeTest is a better, more generic name for the test's purpose, we could change its name in a separate change. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1953904252
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v6]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Review feedback - Rename back to LdapSSLHandshakeFailureTest to ease reviewing - Merge branch 'master' into JDK-8325579 - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - Enhance test - JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/d8d1d6db..ec6579d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=04-05 Stats: 5175 lines in 157 files changed: 1842 ins; 1842 del; 1491 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v5]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Typo - Merge branch 'master' into JDK-8325579 - Rename test and refine comment - Enhance test - JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/88ae0b27..d8d1d6db Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=03-04 Stats: 1085 lines in 40 files changed: 842 ins; 125 del; 118 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v4]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request incrementally with one additional commit since the last revision: Rename test and refine comment - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/c0dee34c..88ae0b27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=02-03 Stats: 683 lines in 2 files changed: 344 ins; 339 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v3]
On Thu, 15 Feb 2024 15:11:15 GMT, Christoph Langer wrote: >> During analysing a customer case I figured out that we have an inconsistency >> between documentation and actual behavior in class >> com.sun.jndi.ldap.Connection. The [method documentation of >> com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) >> states: "If a timeout is supplied but unconnected sockets are not supported >> then the timeout is ignored and a connected socket is created." >> >> This, however does not happen. If a SocketFactory would not support >> unconnected sockets, it would likely throw a SocketException in >> [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). >> And since [the >> code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) >> does not check for this behavior, a connection with timeout value through a >> SocketFactory that does not support unconnected sockets would simply fail >> with an IOException. >> >> So we should either make the code adhere to what is documented or adapt the >> documentation to the actual behavior. >> >> I hereby try to fix the connect coding. Alternatively, we could also adapt >> the description - I have no strong opinion. What do the experts suggest? > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Enhance test > - JDK-8325579 So, I've updated the PR. Sorry for force-pushing, hope you didn't review the code in detail yet. The new version updates module-info as requested. I also enhanced the test `LdapSSLHandshakeFailureTest.java` and added variants that exercise a Socket Factory which does not support unconnected sockets. Two of the test variants would fail with the current JDK. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1946315957
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v3]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Enhance test - JDK-8325579 - Changes: https://git.openjdk.org/jdk/pull/17797/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=02 Stats: 235 lines in 3 files changed: 128 ins; 29 del; 78 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket [v2]
> During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Christoph Langer has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: JDK-8325579 - Changes: - all: https://git.openjdk.org/jdk/pull/17797/files - new: https://git.openjdk.org/jdk/pull/17797/files/419f96f5..48318eb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
On Fri, 9 Feb 2024 21:29:28 GMT, Christoph Langer wrote: > During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Hi Aleksei, thanks for taking a look. Yes, I'm working on extending the test. Stay tuned. 😄 Cheers Christoph - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1944018942
RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
During analysing a customer case I figured out that we have an inconsistency between documentation and actual behavior in class com.sun.jndi.ldap.Connection. The [method documentation of com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) states: "If a timeout is supplied but unconnected sockets are not supported then the timeout is ignored and a connected socket is created." This, however does not happen. If a SocketFactory would not support unconnected sockets, it would likely throw a SocketException in [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). And since [the code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) does not check for this behavior, a connection with timeout value through a SocketFactory that does not support unconnected sockets would simply fail with an IOException. So we should either make the code adhere to what is documented or adapt the documentation to the actual behavior. I hereby try to fix the connect coding. Alternatively, we could also adapt the description - I have no strong opinion. What do the experts suggest? - Commit messages: - JDK-8325579 Changes: https://git.openjdk.org/jdk/pull/17797/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17797&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325579 Stats: 38 lines in 1 file changed: 17 ins; 12 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/17797.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797 PR: https://git.openjdk.org/jdk/pull/17797
Re: RFR: 8323782: Race: Thread::interrupt vs. AbstractInterruptibleChannel.begin [v2]
On Wed, 17 Jan 2024 15:38:22 GMT, Richard Reingruber wrote: >> Set `interrupted` in `Thread::interrupt` before reading `nioBlocker` for >> correct (Dekker scheme) synchronization with concurrent execution of >> [`AbstractInterruptibleChannel::begin`](https://github.com/openjdk/jdk/blob/59062402b9c5ed5612a13c1c40eb22cf1b97c41a/src/java.base/share/classes/java/nio/channels/spi/AbstractInterruptibleChannel.java#L176). >> >> The change passed our CI functional testing: JTReg tests: tier1-4 of hotspot >> and jdk. All of Langtools and jaxp. SPECjvm2008, SPECjbb2015, Renaissance >> Suite, and SAP specific tests. >> Testing was done with fastdebug and release builds on the main platforms and >> also on Linux/PPC64le and AIX. > > Richard Reingruber has updated the pull request incrementally with one > additional commit since the last revision: > > Review Alan test/jdk/java/nio/channels/Selector/LotsOfInterrupts.java line 2: > 1: /* > 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. I guess you should credit SAP here. - PR Review Comment: https://git.openjdk.org/jdk/pull/17444#discussion_r1456496044
Re: RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits [v2]
On Tue, 9 Jan 2024 10:22:40 GMT, Eirik Bjørsnøs wrote: >> This PR suggests that `Files.setPosixPermissions`as implemented by >> `ZipFileSystem` should preserve the leading seven bits of the 'external file >> attributes' field. These bits contain the 'file type', 'setuid', 'setgid', >> and 'sticky' bits. These are unrelated to permissions and should not be >> modified by this operation. >> >> The fix is to update `Entry.readCEN` to read all 16 bits instead of just the >> trailing 12 and to update `ZipFileSystem.setPermissions` to preserve the >> leading 7 bits when updating the trailing 9 permission-related bits of the >> `Entry.posixPerms` field. >> >> The PR adds a new test `TestPosix.preserveRemainingBits()` which verifies >> that the leading 7 bits are not affected by `Files.setPosixPermissions`. >> This test also verifies that operations not related to POSIX, such as >> Files.setLastModifiedTime does not affect the 'external file attributes' >> value. >> >> Note that this PR does not aim to preserve the leading seven bits for the >> case when `Files.setPosixPermissions` is called with a `null` permission >> set. (The implementation currently interprets this as a signal that the >> 'external file attributes' should not be populated and the 'version made >> by' OS will be MSDOS instead of Unix) > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Verify that ZipFileSystem preserves 'external file attribute' bits when > performing operations unrelated to POSIX, such as Files.setLastModifiedTime. > - Merge branch 'master' into zipfs-preserve-external-file-attrs > - Preserve non-permission 'external file attributes' bits when setting posix > permissions Looks correct to me, too. I also went over the changes to the test and it makes sense. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17170#pullrequestreview-1812103908
Re: RFR: 8322772: Clean up code after JDK-8322417
On Fri, 29 Dec 2023 13:44:27 GMT, Christoph Langer wrote: > In the review of the PR for JDK-8322417 it was noted that a fully qualified > class name "java.util.Arrays" is unnecessary but it was forgotten to clean it > up prior to integration. Integrating under trivial rule. - PR Comment: https://git.openjdk.org/jdk/pull/17203#issuecomment-1872353704
Integrated: 8322772: Clean up code after JDK-8322417
On Fri, 29 Dec 2023 13:44:27 GMT, Christoph Langer wrote: > In the review of the PR for JDK-8322417 it was noted that a fully qualified > class name "java.util.Arrays" is unnecessary but it was forgotten to clean it > up prior to integration. This pull request has now been integrated. Changeset: 32d80e2c Author: Christoph Langer URL: https://git.openjdk.org/jdk/commit/32d80e2caf6063b58128bd5f3dc87b276f3bd0cb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8322772: Clean up code after JDK-8322417 Reviewed-by: mdoerr, goetz, mbaesken, vtewari - PR: https://git.openjdk.org/jdk/pull/17203
RFR: 8322772: Clean up code after JDK-8322417
In the review of the PR for JDK-8322417 it was noted that a fully qualified class name "java.util.Arrays" is unnecessary but it was forgotten to clean it up prior to integration. - Commit messages: - JDK-8322417 Changes: https://git.openjdk.org/jdk/pull/17203/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17203&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322772 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17203.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17203/head:pull/17203 PR: https://git.openjdk.org/jdk/pull/17203
Re: RFR: JDK-8317307: test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails with ConnectException: Connection timed out: no further information
On Fri, 1 Dec 2023 13:46:14 GMT, Matthias Baesken wrote:
> On Windows we recently run into this error rather often in the test
> LdapPoolTimeoutTest.java :
>
> MSG RTE: javax.naming.CommunicationException: example.com:1234 [Root
> exception is java.net.ConnectException: Connection timed out: no further
> information]
> MSG: Connect timed out
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> MSG: Timed out waiting for lock
> MSG: Timed out waiting for lock
> MSG: Timed out waiting for lock
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> MSG: Timeout exceeded while waiting for a connection: 26984ms
> java.lang.Exception: failures: 1
> at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:104)
> at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:58)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
> at java.base/java.lang.Thread.run(Thread.java:1570)
>
> We should extend the accepted exception message strings and also also
> 'Connection timed out' .
Looks good and makes sense to accept this case as well since we see it occuring
in real life.
Maybe move the `|| msg.contains("Connection timed out")` into the next line.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16922#pullrequestreview-1759855858
Integrated: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
On Wed, 22 Nov 2023 15:59:38 GMT, Christoph Langer wrote: > java/lang/invoke/lambda/LambdaFileEncodingSerialization.java is already > problem listed on linux-x64. However, the issue is not processor specific. We > see the failure on linux on other architectures as well. This pull request has now been integrated. Changeset: f6e5559a Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/f6e5559ae9d1c8b84b31af5d36e93b43e7731ba5 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all Reviewed-by: mbaesken - PR: https://git.openjdk.org/jdk/pull/16784
RFR: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
java/lang/invoke/lambda/LambdaFileEncodingSerialization.java is already problem listed on linux-x64. However, the issue is not processor specific. We see the failure on linux on other architectures as well. - Commit messages: - Update ProblemList.txt Changes: https://git.openjdk.org/jdk/pull/16784/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16784&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320601 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16784.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16784/head:pull/16784 PR: https://git.openjdk.org/jdk/pull/16784
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Tue, 5 Sep 2023 13:52:09 GMT, Roger Riggs wrote: > A bit late due to a US holiday. Looks good. Thanks 🙇 - PR Comment: https://git.openjdk.org/jdk/pull/15222#issuecomment-1706695064
Integrated: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Thu, 10 Aug 2023 09:54:43 GMT, Christoph Langer wrote: > On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run > as user that is member of the Administrators group. In that case new files > are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks > the assumptions of the test's whoami check. My suggestion is to cater for > this case and don't fail the test but write a warning message to stdout that > a whoami check is not correctly possible. This pull request has now been integrated. Changeset: 69c9ec92 Author:Christoph Langer URL: https://git.openjdk.org/jdk/commit/69c9ec92d04a399946b2157690a1dc3fec517329 Stats: 46 lines in 1 file changed: 31 ins; 10 del; 5 mod 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges Reviewed-by: mbaesken, azeller - PR: https://git.openjdk.org/jdk/pull/15222
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Fri, 1 Sep 2023 08:32:20 GMT, Christoph Langer wrote: >> On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run >> as user that is member of the Administrators group. In that case new files >> are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks >> the assumptions of the test's whoami check. My suggestion is to cater for >> this case and don't fail the test but write a warning message to stdout that >> a whoami check is not correctly possible. > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Use System.getProperty(user.name) for the Windows Adminstrators case > - Merge branch 'master' into JDK-8314094 > - JDK-8314094 OK, the latest update seems to work. I'll integrate tomorrow, unless I hear concerns. - PR Comment: https://git.openjdk.org/jdk/pull/15222#issuecomment-1704820273
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
> On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run > as user that is member of the Administrators group. In that case new files > are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks > the assumptions of the test's whoami check. My suggestion is to cater for > this case and don't fail the test but write a warning message to stdout that > a whoami check is not correctly possible. Christoph Langer has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Use System.getProperty(user.name) for the Windows Adminstrators case - Merge branch 'master' into JDK-8314094 - JDK-8314094 - Changes: - all: https://git.openjdk.org/jdk/pull/15222/files - new: https://git.openjdk.org/jdk/pull/15222/files/4e5cbf82..db6d3d14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=00-01 Stats: 27267 lines in 894 files changed: 17633 ins; 3604 del; 6030 mod Patch: https://git.openjdk.org/jdk/pull/15222.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15222/head:pull/15222 PR: https://git.openjdk.org/jdk/pull/15222
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges [v2]
On Thu, 31 Aug 2023 15:08:34 GMT, Roger Riggs wrote:
>> The problem with the environment variables is, that jtreg only passes very
>> few of them down to the testee process - USERDOMAIN and USERNAME are not
>> part of these as far as I know.
>
> ok, more overhead than value in the suggestion.
> If its windows, strip off the domain (before "/") and compare.
> If that doesn't work then just drop the username compare on windows.
After verifying that System.getenv yields empty results for USERDOMAIN and
USERNAME, I updated the change to use System.getProperty("user.name") in the
Windows Administrators case. Let's see how testing goes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15222#discussion_r1312738642
Re: RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Mon, 28 Aug 2023 05:24:09 GMT, Arno Zeller wrote:
>> I think you might use System.getProperty("user.name"). But I am not sure
>> about domain names of users on Windows.
>> I am also not sure why the user name is currently determined by creating a
>> file - there might be a reason for this that is not obvious to me.
>
> It seems that ProcessHandle.info() returns **DOMAIN/USERNAME** on Windows
> but System.getProperty("user.name") only the **USERNAME**.
> You can get **DOMAIN** and **USERNAME** on **Windows** by calling:
> com.sun.security.auth.module.NTSystem NTSystem = new
> com.sun.security.auth.module.NTSystem();
> String user = NTSystem.getName();
> String domain = NTSystem.getDomain();
Yes, I think using System.getProperty("user.name") is brittle as well. If we'd
use `com.sun.security.auth.module.NTSystem`, we would introduce the dependency
to another module - `jdk.security.auth`. Not sure, whether this is a good
option.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15222#discussion_r1308412488
RFR: 8314094: java/lang/ProcessHandle/InfoTest.java fails on Windows when run as user with Administrator privileges
On Windows, the test java/lang/ProcessHandle/InfoTest.java can fail when run as user that is member of the Administrators group. In that case new files are not owned by the user but instead by BUILTIN\ADMINISTRATORS. This breaks the assumptions of the test's whoami check. My suggestion is to cater for this case and don't fail the test but write a warning message to stdout that a whoami check is not correctly possible. - Commit messages: - JDK-8314094 Changes: https://git.openjdk.org/jdk/pull/15222/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15222&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314094 Stats: 21 lines in 1 file changed: 14 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15222.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15222/head:pull/15222 PR: https://git.openjdk.org/jdk/pull/15222
[jdk21] Integrated: 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)]
On Tue, 1 Aug 2023 21:57:17 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8311822](https://bugs.openjdk.org/browse/JDK-8311822), commit > [d1cc2782](https://github.com/openjdk/jdk/commit/d1cc2782606e8a3cfead9055aa845e48e851edd4) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Per Minborg on 24 Jul 2023 and > was reviewed by Jorn Vernee. > > > Test fix, so qualifying for jdk21 under RDP2 rules. > > Thanks! This pull request has now been integrated. Changeset: 610812d4 Author:Christoph Langer URL: https://git.openjdk.org/jdk21/commit/610812d4743d9ef9f6e92f7f2eea179fbdf81387 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)] Reviewed-by: mbaesken Backport-of: d1cc2782606e8a3cfead9055aa845e48e851edd4 - PR: https://git.openjdk.org/jdk21/pull/157
[jdk21] RFR: 8311822: AIX : test/jdk/java/foreign/TestLayouts.java fails because of different output - expected [[i4](struct)] but found [[I4](struct)]
Hi all, This pull request contains a backport of [JDK-8311822](https://bugs.openjdk.org/browse/JDK-8311822), commit [d1cc2782](https://github.com/openjdk/jdk/commit/d1cc2782606e8a3cfead9055aa845e48e851edd4) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Per Minborg on 24 Jul 2023 and was reviewed by Jorn Vernee. Test fix, so qualifying for jdk21 under RDP2 rules. Thanks! - Commit messages: - Backport d1cc2782606e8a3cfead9055aa845e48e851edd4 Changes: https://git.openjdk.org/jdk21/pull/157/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=157&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8311822 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk21/pull/157.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/157/head:pull/157 PR: https://git.openjdk.org/jdk21/pull/157
Re: [jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Fri, 28 Jul 2023 09:10:20 GMT, Alan Bateman wrote: > JDK-8308609 I added a comment on https://bugs.openjdk.org/browse/JDK-8303498, cc @offamitkumar - PR Comment: https://git.openjdk.org/jdk21/pull/149#issuecomment-1655513822
Re: [jdk21] RFR: 8313260: JDK21: ProblemList java/lang/ScopedValue/StressStackOverflow.java on linux-x86
On Thu, 27 Jul 2023 18:21:45 GMT, Sergey Bylokhov wrote: > This patch adds the java/lang/ScopedValue/StressStackOverflow.java to the > problem list for linux-x86 where it intermittently fails on a GA, ex: > https://github.com/openjdk/jdk21/pull/148 > > This is only for JDK 21, test passes on JDK 22. This is good. Thanks for doing it. I guess we should backport the real fixes to jdk21u. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk21/pull/149#pullrequestreview-1551465386
Re: RFR: JDK-8310550: Adjust references to rt.jar [v4]
On Wed, 5 Jul 2023 15:01:52 GMT, Matthias Baesken wrote: >> There are a few references to rt.jar in comments and in the codebase itself. >> Some of them might be removed or adjusted. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjust comments Fine from my end now. Just one minor nit left. 😄 src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java line 196: > 194: > 195: /** > 196: * Set whether or not to use ct.sym as an alternate to the current > runtime You should bring back the period at the end of the sentence. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514740197 PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253244166
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Thu, 22 Jun 2023 09:21:29 GMT, Matthias Baesken wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java >> line 196: >> >>> 194: >>> 195: /** >>> 196: * Set whether or not to use ct.sym as an alternate >> >> As an alternate to what? This needs something else. > > should "to the image modules files" be used instead ? maybe "... to the current runtime."? - PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253139297
Re: RFR: JDK-8310550: Adjust references to rt.jar [v3]
On Fri, 30 Jun 2023 11:37:10 GMT, Matthias Baesken wrote:
>> There are a few references to rt.jar in comments and in the codebase itself.
>> Some of them might be removed or adjusted.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove import
Looks good overall. I made a few suggestions.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach024/TestDescription.java
line 40:
> 38: * Agent's JAR file contains modified class
> java.util.TooManyListenersException (it is assumed
> 39: * that this class isn't loaded before agent is loaded), agent
> instantiates TooManyListenersException
> 40: * and checks that non-modified version of this class was loaded from
> jdk image (not from agent's JAR).
"from the jdk image"
test/jdk/com/sun/tools/attach/ProviderTest.java line 110:
> 108: public static void main(String args[]) throws Exception {
> 109: // deal with internal builds where classes are loaded from
> the
> 110: // 'classes' directory rather than the image modules file
"... rather than the runtime image"
test/langtools/tools/javap/4798312/JavapShouldLoadClassesFromRTJarTest.java
line 27:
> 25: * @test
> 26: * @bug 4798312
> 27: * @summary In Windows, javap doesn't load classes from image
"... from the runtime image"
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14593#pullrequestreview-1514576016
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253140142
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253141204
PR Review Comment: https://git.openjdk.org/jdk/pull/14593#discussion_r1253142105
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v3]
On Thu, 22 Jun 2023 11:51:15 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> move javaPath into caller
Looks good to me now.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1493063728
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work [v2]
On Thu, 22 Jun 2023 09:53:29 GMT, Matthias Baesken wrote:
>> Currently, a number of tests fail on macOS because they miss the core file
>> (e.g. serviceability/sa/TestJmapCore.java).
>> The reason is that configure detects on some setups that codesign does not
>> work ("checking if debug mode codesign is possible... no) .
>> So adding the needed entitlement to generate cores is not done in the build.
>> This is currently not checked later in the tests.
>> But without the entitlement, a core is not generated.
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Some adjustments
Looks a bit better. But I think instead of adding a Utils.javaPath() method,
you can do all this path handling in Platform.launchCodesignOnJavaBinary().
Then even more code would be shared.
test/lib/jdk/test/lib/Platform.java line 263:
> 261: }
> 262:
> 263: private static codesignProcess launchCodesignOnJavaBinary(String
> javaFileName) {
That can't work... should be `private static Process` 😉
-
Changes requested by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1492927024
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1238354879
Re: RFR: JDK-8310380: Handle problems in core-related tests on macOS when codesign tool does not work
On Tue, 20 Jun 2023 13:23:16 GMT, Matthias Baesken wrote:
> Currently, a number of tests fail on macOS because they miss the core file
> (e.g. serviceability/sa/TestJmapCore.java).
> The reason is that configure detects on some setups that codesign does not
> work ("checking if debug mode codesign is possible... no) .
> So adding the needed entitlement to generate cores is not done in the build.
> This is currently not checked later in the tests.
> But without the entitlement, a core is not generated.
I made a few review suggestions. Does the symptom happen on both, arm64 and x64?
test/lib/jdk/test/lib/Platform.java line 267:
> 265: // Find the path to the java binary.
> 266: String jdkPath = System.getProperty("java.home");
> 267: Path javaPath = Paths.get(jdkPath + "/bin/java");
You could do it more concisely:
Path javaPath = Paths.get(System.getProperty("java.home") + "/bin/java");
if (Files.notExists(javaPath)) {
throw new FileNotFoundException("Could not find file " +
javaPath.toAbsolutePath().toString());
test/lib/jdk/test/lib/Platform.java line 274:
> 272:
> 273: // Run codesign on the java binary.
> 274: ProcessBuilder pb = new ProcessBuilder("codesign", "--display",
> "--verbose", javaFileName);
And then here 'ProcessBuilder pb = new ProcessBuilder("codesign", "--display",
"--verbose", javaPath.toAbsolutePath().toString());`
test/lib/jdk/test/lib/Platform.java line 277:
> 275: pb.redirectErrorStream(true); // redirect stderr to stdout
> 276: Process codesignProcess = pb.start();
> 277: BufferedReader is = new BufferedReader(new
> InputStreamReader(codesignProcess.getInputStream()));
Maybe do the BufferedReader stuff in a try-with-resources and then return false
instead of potentially throwing an IOException?
test/lib/jdk/test/lib/Platform.java line 280:
> 278: String line;
> 279: while ((line = is.readLine()) != null) {
> 280: System.out.println("STDOUT: " + line);
This output is for every line seems too much. Maybe just print the lines where
you find "Info.plist=not bound" or "Info.plist entries="?
test/lib/jdk/test/lib/util/CoreUtils.java line 153:
> 151: throw new SkippedException("Cannot produce core file
> with hardened binary on OSX 10.15 and later");
> 152: }
> 153: } else {
Maybe you could do just one case here:
`else if (!Platform.hasPlistEntriesOSX()) {`...
-
PR Review: https://git.openjdk.org/jdk/pull/14562#pullrequestreview-1491050995
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237190071
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237190832
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237191976
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237194492
PR Review Comment: https://git.openjdk.org/jdk/pull/14562#discussion_r1237184922
Re: RFR: JDK-8308300: enhance exceptions in MappedMemoryUtils.c [v2]
On Fri, 19 May 2023 09:46:59 GMT, Matthias Baesken wrote: >> MappedMemoryUtils.c can generate exceptions like those : >> java.io.UncheckedIOException: java.io.IOException: Invalid argument >>at >> java.base/java.nio.MappedMemoryUtils.force(MappedMemoryUtils.java:105) >>at java.base/java.nio.Buffer$2.force(Buffer.java:890) >>at >> java.base/jdk.internal.misc.ScopedMemoryAccess.forceInternal(ScopedMemoryAccess.java:317) >>at >> java.base/jdk.internal.misc.ScopedMemoryAccess.force(ScopedMemoryAccess.java:305) >>at >> java.base/jdk.internal.foreign.MappedMemorySegmentImpl.force(MappedMemorySegmentImpl.java:92) >>at >> TestByteBuffer.testMappedSegmentAsByteBuffer(TestByteBuffer.java:327) >> >> (we see this for example on AIX); there is some room for improvement, at >> least the info should be added that msync failed and caused this exception. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Change comments and madvise exception text Marked as reviewed by clanger (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14054#pullrequestreview-1434195594
[jdk20] Integrated: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
On Wed, 11 Jan 2023 09:21:18 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit > [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Justin Lu on 4 Jan 2023 and was > reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. > > Thanks! This pull request has now been integrated. Changeset: 752a3701 Author:Christoph Langer URL: https://git.openjdk.org/jdk20/commit/752a37016f49ef8f2405dd74f96f58f80d606cd3 Stats: 8 lines in 3 files changed: 1 ins; 2 del; 5 mod 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR Reviewed-by: naoto Backport-of: 3b374c0153950ab193f3a188b57d3404b4ce2fe2 - PR: https://git.openjdk.org/jdk20/pull/96
Re: [jdk20] RFR: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
On Wed, 11 Jan 2023 17:01:26 GMT, Naoto Sato wrote: >> Hi all, >> >> This pull request contains a backport of >> [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit >> [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) >> from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. >> >> The commit being backported was authored by Justin Lu on 4 Jan 2023 and was >> reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. >> >> Thanks! > > Looks good. Thanks for confirming, @naotoj - PR: https://git.openjdk.org/jdk20/pull/96
[jdk20] RFR: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
Hi all, This pull request contains a backport of [JDK-8299439](https://bugs.openjdk.org/browse/JDK-8299439), commit [3b374c01](https://github.com/openjdk/jdk/commit/3b374c0153950ab193f3a188b57d3404b4ce2fe2) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Justin Lu on 4 Jan 2023 and was reviewed by Naoto Sato, Lance Andersen and Jaikiran Pai. Thanks! - Commit messages: - Backport 3b374c0153950ab193f3a188b57d3404b4ce2fe2 Changes: https://git.openjdk.org/jdk20/pull/96/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=96&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299439 Stats: 8 lines in 3 files changed: 1 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jdk20/pull/96.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/96/head:pull/96 PR: https://git.openjdk.org/jdk20/pull/96
Re: RFR: 8290059: Do not use std::thread in panama tests
On Fri, 22 Jul 2022 15:09:13 GMT, Christoph Langer wrote: >> This patch removes the use of std::thread from the `java.lang.foreign` >> tests, and switches to the OS specific thread APIs, in order to change >> things such as the stack size on some platforms where this is required in >> the future (see the JBS issue). >> >> This is done by adding a small header-only library that exposes a function >> to run code in freshly spawned threads (`run_async`). >> >> Testing: Running the affected tests on platforms that implement the linker. > > @JornVernee thanks for doing this so quickly. I suggest undoing > https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7 > in this change, too. If you update this PR I can run it through our Alpine > test pipeline. > @RealCLanger Note that I'm not setting the stack size of the thread in this > patch. I'm just using the defaults. From the discussion on the bug, I don't > think this sufficient to make the tests pass on Apline/MuslC. > > I avoided getting into that since I don't have ready access to an > Alpine/MuslC testing environment atm. So, I've left setting the stack size on > MuslC, and re-enabling the tests for someone that does. Hopefully this patch > is enough to get that going easily. OK, thanks for clarifying that. @tstuefe, maybe you want to have a look after this fix is in? - PR: https://git.openjdk.org/jdk/pull/9599
Re: RFR: 8290059: Do not use std::thread in panama tests
On Thu, 21 Jul 2022 18:48:14 GMT, Jorn Vernee wrote: > This patch removes the use of std::thread from the `java.lang.foreign` tests, > and switches to the OS specific thread APIs, in order to change things such > as the stack size on some platforms where this is required in the future (see > the JBS issue). > > This is done by adding a small header-only library that exposes a function to > run code in freshly spawned threads (`run_async`). > > Testing: Running the affected tests on platforms that implement the linker. @JornVernee thanks for doing this so quickly. I suggest undoing https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7 in this change, too. If you update this PR I can run it through our Alpine test pipeline. - PR: https://git.openjdk.org/jdk/pull/9599
[jdk19] Integrated: 8290460: Alpine: disable some panama tests that rely on std::thread
On Fri, 22 Jul 2022 07:04:29 GMT, Christoph Langer wrote: > Hi all, > > This pull request contains a backport of > [JDK-8290460](https://bugs.openjdk.org/browse/JDK-8290460), commit > [d7f0de27](https://github.com/openjdk/jdk/commit/d7f0de272c85ee8d0890c9d61e10065b618b69d7) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > It is a testfix, so should be good to submit within RDP2. The issue is > observed in SAP's CI. > > Thanks! This pull request has now been integrated. Changeset: c29242eb Author:Christoph Langer URL: https://git.openjdk.org/jdk19/commit/c29242ebb0cfd3eaa56240664af607c02a11324e Stats: 3 lines in 2 files changed: 3 ins; 0 del; 0 mod 8290460: Alpine: disable some panama tests that rely on std::thread Backport-of: d7f0de272c85ee8d0890c9d61e10065b618b69d7 - PR: https://git.openjdk.org/jdk19/pull/151
