Re: RFR: 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent
On Fri, 18 Aug 2023 23:14:20 GMT, Brian Burkhalter wrote: > Revise some verbiage about the `RandomAccessFile(String,String)` constructor > so that the string name of a file is referred to as _pathname string_ for > consistency with `java.io.File`. Looks OK to me - Marked as reviewed by vtewari (Committer). PR Review: https://git.openjdk.org/jdk/pull/15351#pullrequestreview-1585469761
Re: RFR: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
On Fri, 18 Aug 2023 08:18:24 GMT, Viktor Klang wrote:
>> The usual tiny improvements, with no guarantee that the intermittent test
>> failure is actually fixed.
>
> test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 70:
>
>> 68: fair, i, threadCount, j));
>> 69: }
>> 70: for (Thread t : ts) t.join();
>
> @Martin-Buchholz Makes a lot of sense to make sure that all threads have
> terminated before exiting the test
There's a lot of infrastructure in JSR166TestCase.java to address this. Some
of it could be librarified for use by other openjdk tests.
* All code not running in the main test thread (manually spawned threads
* or the common fork join pool) must be checked for failure (and completion!).
* Mechanisms that can be used to ensure this are:
*
* Signalling via a synchronizer like AtomicInteger or CountDownLatch
*that the task completed normally, which is checked before returning from
*the test method in the main thread.
* Using the forms {@link #threadFail}, {@link #threadAssertTrue},
*or {@link #threadAssertNull}, (not {@code fail}, {@code assertTrue}, etc.)
*Only the most typically used JUnit assertion methods are defined
*this way, but enough to live with.
* Recording failure explicitly using {@link #threadUnexpectedException}
*or {@link #threadRecordFailure}.
* Using a wrapper like CheckedRunnable that uses one the mechanisms
above.
*
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1299029889
Re: RFR: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
On Fri, 18 Aug 2023 08:15:58 GMT, Viktor Klang wrote:
>> The usual tiny improvements, with no guarantee that the intermittent test
>> failure is actually fixed.
>
> test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 41:
>
>> 39: throws Throwable
>> 40: {
>> 41: int threadCount = ThreadLocalRandom.current().nextInt(2, 8);
>
> @Martin-Buchholz I'm a bit weary about making the number of threads
> random—might make it tricky to get to reproduce if the test fails?
Suppose the test fails only with a specific number of threads. I would want to
know that! At least once in the past a j.u.c. bug has been found due to
randomization of concurrency level in a test. I can't understand the current
industry obsession with deterministic tests, except that too many
organizations don't commit to hunting down flakes.
Here the error detail has been changed to report the threadcount, in case of
failure.
> test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 55:
>
>> 53: Thread t = new Thread(put);
>> 54: t.start();
>> 55: ts.add(t);
>
> @Martin-Buchholz Any specific reason to add the thread *after* it has been
> started?
I've never considered that the order would matter - so, no reason.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1299024335
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1299027961
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
On Fri, 18 Aug 2023 13:30:54 GMT, Weibing Xiao wrote: >> Please refer to JDK-8314063. >> >> The failure scenario is due to the setting of connection timeout. It is >> either too small or not an optimal value for the system. When the client >> tries to connect to the server with LDAPs protocol. It requires the >> handshake after the socket is created and connected, but it fails due to >> connection timeout and leaves the socket open. It is not closed properly due >> to the exception handling in the JDK code. >> >> The change is adding a try/catch block and closing the socket in the catch >> block, and the format of the code got changed consequently. > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > re-write close method 1) Refactor the code again, and simplify the code to handle the setting of connection time 2) Add more test cases 3) Bring back the pre-created keystore. I could not find a good example to create the key store at runtime. - PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1684688234
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v7]
> Please refer to JDK-8314063. > > The failure scenario is due to the setting of connection timeout. It is > either too small or not an optimal value for the system. When the client > tries to connect to the server with LDAPs protocol. It requires the handshake > after the socket is created and connected, but it fails due to connection > timeout and leaves the socket open. It is not closed properly due to the > exception handling in the JDK code. > > The change is adding a try/catch block and closing the socket in the catch > block, and the format of the code got changed consequently. Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: refactor the code and test cases - Changes: - all: https://git.openjdk.org/jdk/pull/15294/files - new: https://git.openjdk.org/jdk/pull/15294/files/ac72c3d8..e9e0497f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15294=06 - incr: https://webrevs.openjdk.org/?repo=jdk=15294=05-06 Stats: 121 lines in 3 files changed: 48 ins; 36 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/15294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15294/head:pull/15294 PR: https://git.openjdk.org/jdk/pull/15294
Re: RFR: 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent
On Fri, 18 Aug 2023 23:14:20 GMT, Brian Burkhalter wrote: > Revise some verbiage about the `RandomAccessFile(String,String)` constructor > so that the string name of a file is referred to as _pathname string_ for > consistency with `java.io.File`. LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/15351#pullrequestreview-1585330641
RFR: 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent
Revise some verbiage about the `RandomAccessFile(String,String)` constructor so that the string name of a file is referred to as _pathname string_ for consistency with `java.io.File`. - Commit messages: - 8219567: Name of first parameter of RandomAccessFile(String,String) is inconsistent Changes: https://git.openjdk.org/jdk/pull/15351/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15351=00 Issue: https://bugs.openjdk.org/browse/JDK-8219567 Stats: 13 lines in 1 file changed: 0 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/15351.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15351/head:pull/15351 PR: https://git.openjdk.org/jdk/pull/15351
Re: RFR: 8314236: Overflow in Collections.rotate [v2]
On Wed, 16 Aug 2023 21:38:09 GMT, Stuart Marks wrote: >> Thanks! Done > > @nikita-sakharin > > Thanks for finding this bug and offering to fix it! (And @shipilev thanks for > your assistance on this.) > > Putting the test into a separate JVM will work, but I don't think it's > necessary to actually allocate the space. The test is only testing the > indexes sent to `get` and `set` on the list, and it doesn't actually verify > the contents of the list. (Presumably that's done by other tests.) Therefore > it should be possible to create a "virtual" list of a given size that checks > that the indexes are all in bounds but that doesn't actually store any > elements. It should be fairly straightforward to do this by subclassing > AbstractList and overriding a few methods. > > The advantage of not actually allocating 4G of memory is that it makes it > easier to run a bunch of cases that test the boundary conditions. In fact I'd > like to see that in the test, as opposed to testing this one case. @stuart-marks Thanks for your review! I rewrote the test according with your advise. All checks passed successfully. Could you review the changes, please? - PR Review Comment: https://git.openjdk.org/jdk/pull/15270#discussion_r1297105328
RFR: 8314604: j.text.DecimalFormat behavior regarding patterns is not clear
Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8314607)
which clarifies the behavior of patterns in regards to the max integer digits
in j.text.DecimalFormat.
The current specification (of `applyPattern`) states that patterns do not set
the value of max integer digits. This is incorrect, these methods/constructors
do set a value for the max integer digits. If the pattern is in scientific
notation, the max integer digits value is derived from the pattern. Otherwise,
the pattern is ignored, and the limit is set to `Integer.MAX_VALUE`.
See below,
DecimalFormat df = new DecimalFormat();
df.applyPattern("000.000E0");
df.getMaximumIntegerDigits(); // ==> 3
df.applyPattern("000.000");
df.getMaximumIntegerDigits(); // ==> 2147483647
DecimalFormat df = new DecimalFormat("000.000");
df.getMaximumIntegerDigits(); // ==> 2147483647
DecimalFormat df = new DecimalFormat("000.000E0");
df.getMaximumIntegerDigits(); // ==> 3
Method descriptions should be fixed, and the relevant constructors need to
mention the behavior as well.
-
Commit messages:
- Include constructors
- Re-clarify spec
- Init
Changes: https://git.openjdk.org/jdk/pull/15349/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=15349=00
Issue: https://bugs.openjdk.org/browse/JDK-8314604
Stats: 28 lines in 1 file changed: 22 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/15349.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15349/head:pull/15349
PR: https://git.openjdk.org/jdk/pull/15349
Re: RFR: 8314236: Overflow in Collections.rotate [v2]
On Tue, 15 Aug 2023 09:54:48 GMT, Aleksey Shipilev wrote:
>> Nikita Sakharin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8314236: change bug number and summary
>
> src/java.base/share/classes/java/util/Collections.java line 810:
>
>> 808:
>> 809: private static void rotate1(final List list, int distance) {
>> 810: final int size = list.size();
>
> Let's keep the style, and do a focused fix: skip adding `final` here.
> (`final` mostly matters for fields).
Done. In order to be consistent I also removed `final` for `bound` variable.
> src/java.base/share/classes/java/util/Collections.java line 813:
>
>> 811: if (size == 0)
>> 812: return;
>> 813: distance %= size;
>
> Same, let's keep the original style.
Done
> test/jdk/java/util/Collections/RotateHuge.java line 27:
>
>> 25: * @test
>> 26: * @bug 8314236
>> 27: * @summary Overflow in Collections.rotate
>
> Since this test takes >4G of heap to hold the list with compressed oops, and
> >8G of heap without compressed oops, we need to run it in a separate VM with
> enough headroom, something like this:
>
>
> * @test
> * @bug 8314236
> * @summary Overflow in Collections.rotate
> * @requires (sun.arch.data.model == "64" & os.maxMemory >= 16g)
> * @run main/othervm -Xmx12g RotateHuge
Thanks! Done
> test/jdk/java/util/Collections/RotateHuge.java line 40:
>
>> 38: final List list = new ArrayList<>(size);
>> 39: for (int i = 0; i < size; ++i)
>> 40: list.add((byte) 0);
>
> We don't actually need to rely on auto-boxing here, right?
>
> Suggestion:
>
> final Object o = new Object();
> final List list = new ArrayList<>(size);
> for (int i = 0; i < size; i++) {
> list.add(o);
> }
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15270#discussion_r1294467547
PR Review Comment: https://git.openjdk.org/jdk/pull/15270#discussion_r129444
PR Review Comment: https://git.openjdk.org/jdk/pull/15270#discussion_r1294466347
PR Review Comment: https://git.openjdk.org/jdk/pull/15270#discussion_r1294467775
Re: RFR: 8041488: Locale-Dependent List Patterns [v8]
On Thu, 10 Aug 2023 17:31:28 GMT, Naoto Sato wrote:
>> Introducing a new formatting class for locale-dependent list patterns. The
>> class is to provide the functionality from the Unicode Consortium's LDML
>> specification for [list
>> patterns](https://www.unicode.org/reports/tr35/tr35-general.html#ListPatterns).
>> For example, given a list of String as "Monday", "Wednesday", "Friday", its
>> `format` method would produce "Monday, Wednesday, and Friday" in US English.
>> A CSR has also been drafted, and its draft javadoc can be viewed here:
>> https://cr.openjdk.org/~naoto/JDK-8041488-ListPatterns-PR/api.00/java.base/java/text/ListFormat.html
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review comments
src/java.base/share/classes/java/text/ListFormat.java line 199:
> 197:
> 198: /**
> 199: * {@return the list format object for the default
"list format" will read better using the class name `ListFormat`. Here an in
other `getInstance` methods.
As is, in prose it looks like it returns some kind of list.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1298831450
Re: RFR: 8314544: Matrix multiple benchmark using Vector API
On Fri, 18 Aug 2023 03:57:24 GMT, Martin Stypinski wrote:
> Added a bunch of different implementations for Vector API Matrix
> Multiplications:
>
> - Baseline
> - Blocked (Cache Local)
> - FMA
> - Vector API Simple Implementation
> - Vector API Blocked Implementation
>
> Commit was discussed with @PaulSandoz
test/micro/org/openjdk/bench/jdk/incubator/vector/MatrixMultiplicationBenchmark.java
line 33:
> 31:
> 32: import org.openjdk.jmh.annotations.*;
> 33: import org.openjdk.jmh.infra.Blackhole;
Unused import.
test/micro/org/openjdk/bench/jdk/incubator/vector/MatrixMultiplicationBenchmark.java
line 176:
> 174:
> 175: private static float[] newFloatRowMatrix(int size) {
> 176: Random rand = new Random();
Instead we can use the new random generator API e.g,:
var rand = RandomGenerator.getDefault();
This is not super important, but its good practice to try and use more
preferred APIs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15338#discussion_r1298830226
PR Review Comment: https://git.openjdk.org/jdk/pull/15338#discussion_r1298830156
Re: RFR: 8041488: Locale-Dependent List Patterns [v8]
On Thu, 10 Aug 2023 17:31:28 GMT, Naoto Sato wrote:
>> Introducing a new formatting class for locale-dependent list patterns. The
>> class is to provide the functionality from the Unicode Consortium's LDML
>> specification for [list
>> patterns](https://www.unicode.org/reports/tr35/tr35-general.html#ListPatterns).
>> For example, given a list of String as "Monday", "Wednesday", "Friday", its
>> `format` method would produce "Monday, Wednesday, and Friday" in US English.
>> A CSR has also been drafted, and its draft javadoc can be viewed here:
>> https://cr.openjdk.org/~naoto/JDK-8041488-ListPatterns-PR/api.00/java.base/java/text/ListFormat.html
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review comments
src/java.base/share/classes/java/text/ListFormat.java line 96:
> 94: * round-trip with the corresponding formatting. For example, a String
> list
> 95: * "a, b,", "c" will be formatted as "a, b, and c", but may be parsed as
> 96: * "a", "b", "c".
Suggestion:
* round-trip with the corresponding formatting. For example, a two element
String list
* "a, b,", "c" will be formatted as "a, b, and c", but may be parsed as three
elements
* "a", "b", "c".
src/java.base/share/classes/java/text/ListFormat.java line 240:
> 238: * is thrown.
> 239: *
> 240: * Each pattern string is first parsed as follows. Patterns in
> parentheses are optional:
The characters in parentheses are arbitrary and literal strings; not really
"patterns" in the regex sense.
src/java.base/share/classes/java/text/ListFormat.java line 246:
> 244: * end := {0}end_between{1}(end_after)
> 245: * two := (two_before){0}two_between{1}(two_after)
> 246: * three :=
> (three_before){0}three_between{1}three_between{2}(three_after)
The use of "three_between" in 2 positions may mislead the developer to think
they need to be the same string.
Perhaps "three_between1" and "three_between2"? or something to differentiate
them.
src/java.base/share/classes/java/text/ListFormat.java line 250:
> 248: * If parsing of the pattern string for start/middle/end fails, it
> throws an
> 249: * {@code IllegalArgumentException}. If two/three pattern string is
> empty, or
> 250: * fails on parsing, it falls back to
The IllegalArgumentException is thrown for a parse failure on any of the start,
middle, end, two, or three patterns.
The fallback is only if they are null or empty.
A more readable form of "start/middle/end" would be "start, middle, or end".
Here and elsewhere in the javadoc.
src/java.base/share/classes/java/text/ListFormat.java line 262:
> 260: * n > 3: (start_before){0}start_between{1}middle_between{2} ...
> middle_between{m}end_between{n}(end_after)
> 261: *
> 262: * As an example, the following table shows patterns array which is
> equivalent to
Suggestion:
* As an example, the following table shows a pattern array which is
equivalent to
src/java.base/share/classes/java/text/ListFormat.java line 279:
> 277: * 2
> 278: * "{0} and {1}"
> 279: * 3
The use of different terms to identify pattern kinds of 2/two and 3/three may
be confusing.
Unless it is referring to LDML, use "two" and "three" be used consistently.
src/java.base/share/classes/java/text/ListFormat.java line 304:
> 302: * @param patterns array of patterns, not null
> 303: * @throws IllegalArgumentException if the length {@code patterns}
> array is not 5, or
> 304: * any of {@code start}, {@code middle}, {@code end}
> patterns cannot be parsed.
Also throws if two and three patterns can not be parsed.
src/java.base/share/classes/java/text/ListFormat.java line 353:
> 351: return generateMessageFormat(a).format(a, toAppendTo,
> DontCareFieldPosition.INSTANCE);
> 352: } else {
> 353: throw new IllegalArgumentException("The object to format
> should be an Object list");
Suggestion:
throw new IllegalArgumentException("The object to format should be
a List or List[] array");
src/java.base/share/classes/java/text/ListFormat.java line 364:
> 362: * delimiters. For example, a String list {@code "a, b,", "c"} will
> be
> 363: * formatted as {@code "a, b, and c"}, but may be parsed as
> 364: * {@code "a", "b", "c"}.
Suggestion:
* delimiters. For example, a two element String list {@code "a, b,", "c"}
will be
* formatted as {@code "a, b, and c"}, but may be parsed as three elements
* {@code "a", "b", "c"}.
src/java.base/share/classes/java/text/ListFormat.java line 389:
> 387: * use all characters up to the end of the string), and the parsed
> 388: * object is returned. The updated {@code parsePos} can be used to
> 389: * indicate the starting point for the next call to this method.
The "this method" seems too specific to ListFormat.
Suggestion:
* indicate the
Re: RFR: 8314495: Update to use jtreg 7.3.1
On Thu, 17 Aug 2023 07:24:14 GMT, Christian Stein wrote: > Please review the change to update to using jtreg 7.3,1. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > This change set also un-problem-lists tests from > https://github.com/openjdk/jdk/commit/360f65d7b15b327e2f160c42f318945cc6548bda > > This change set also fixes: > - https://bugs.openjdk.org/browse/JDK-8313902 > - https://bugs.openjdk.org/browse/JDK-8313903 > > Testing: tier1-tier5 OK Marked as reviewed by iris (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15323#pullrequestreview-1585041001
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
On Fri, 18 Aug 2023 13:30:54 GMT, Weibing Xiao wrote:
>> Please refer to JDK-8314063.
>>
>> The failure scenario is due to the setting of connection timeout. It is
>> either too small or not an optimal value for the system. When the client
>> tries to connect to the server with LDAPs protocol. It requires the
>> handshake after the socket is created and connected, but it fails due to
>> connection timeout and leaves the socket open. It is not closed properly due
>> to the exception handling in the JDK code.
>>
>> The change is adding a try/catch block and closing the socket in the catch
>> block, and the format of the code got changed consequently.
>
> Weibing Xiao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> re-write close method
Changes requested by aefimov (Reviewer).
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 293:
> 291: } else { // NO SocketFactory
> 292: // create a connected socket without factory
> 293: createConnectionSocket(endpoint, connectTimeout);
A socket returned by `createConnectionSocket` here needs to be saved to the
`socket` variable - currently almost all of the tests that are not using a
custom socket factory - fails with:
Caused by: java.lang.NullPointerException: Cannot invoke
"java.net.Socket.getInputStream()" because "this.sock" is null
at java.naming/com.sun.jndi.ldap.Connection.(Connection.java:238)
-
PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1585010623
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1298730216
Re: [jdk21] RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Fri, 18 Aug 2023 17:25:10 GMT, Ben Taylor wrote: > Backport is clean, all tests in test/jdk/java/util/zip pass. My mistake, will create the PR against 21u. - PR Comment: https://git.openjdk.org/jdk21/pull/173#issuecomment-1684255033
[jdk21] Withdrawn: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Fri, 18 Aug 2023 17:25:10 GMT, Ben Taylor wrote: > Backport is clean, all tests in test/jdk/java/util/zip pass. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk21/pull/173
Re: [jdk21] RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Fri, 18 Aug 2023 17:25:10 GMT, Ben Taylor wrote: > Backport is clean, all tests in test/jdk/java/util/zip pass. It is strange that there is a request for jdk21u but there is no PR for that repo. - PR Comment: https://git.openjdk.org/jdk21/pull/173#issuecomment-1684250803
Re: [jdk21] RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
On Fri, 18 Aug 2023 17:25:10 GMT, Ben Taylor wrote: > Backport is clean, all tests in test/jdk/java/util/zip pass. JDK 21 is at RC, only P1 issues with approval. There is already a fix request for jdk21u. The 21.0.1 release is only a few weeks after JDK 21. - PR Comment: https://git.openjdk.org/jdk21/pull/173#issuecomment-1684240722
[jdk21] RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size)
Backport is clean, all tests in test/jdk/java/util/zip pass. - Commit messages: - Backport 13f6450e2e70df4df8bd882def837fbd5bef1524 Changes: https://git.openjdk.org/jdk21/pull/173/files Webrev: https://webrevs.openjdk.org/?repo=jdk21=173=00 Issue: https://bugs.openjdk.org/browse/JDK-8313765 Stats: 1004 lines in 4 files changed: 986 ins; 4 del; 14 mod Patch: https://git.openjdk.org/jdk21/pull/173.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/173/head:pull/173 PR: https://git.openjdk.org/jdk21/pull/173
Integrated: 8310815: Clarify the name of the main class, services and provider classes in module descriptor
On Wed, 16 Aug 2023 18:42:00 GMT, Mandy Chung wrote: > Clarify the spec of `ModuleDescriptor::mainClass`, `ModuleDescriptor::uses`, > `ModuleDescriptor.Provides::service` and > `ModuleDescriptor.Provides::providers` to return a binary name. > > This PR also fixes JDK-8314449 to update the spec of > `StackTraceElement::getClassName` and the `declaringClass` parameter of the > constructor. This pull request has now been integrated. Changeset: 50a2ce01 Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/50a2ce01f4d1d42d7a537b48a669b5a75a583df5 Stats: 25 lines in 2 files changed: 3 ins; 4 del; 18 mod 8310815: Clarify the name of the main class, services and provider classes in module descriptor 8314449: Clarify the name of the declaring class of StackTraceElement Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/15317
RFR: 8314483: Optionally override copyright header in generated source
In the JDK build we have various build tools that generate source code from data files. For most of these tools, the source files are based on template files, which already have copyright headers, but for some, the complete source file is generated by the tool, which is providing the copyright header programatically. For the latter, we would like to implement an override mechanism in each tool so that we can change the copyright header from a custom makefile. - Commit messages: - JDK-8314483 Changes: https://git.openjdk.org/jdk/pull/15346/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15346=00 Issue: https://bugs.openjdk.org/browse/JDK-8314483 Stats: 43 lines in 4 files changed: 34 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/15346.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15346/head:pull/15346 PR: https://git.openjdk.org/jdk/pull/15346
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v5]
On Fri, 18 Aug 2023 13:20:50 GMT, Weibing Xiao wrote: >> Please refer to JDK-8314063. >> >> The failure scenario is due to the setting of connection timeout. It is >> either too small or not an optimal value for the system. When the client >> tries to connect to the server with LDAPs protocol. It requires the >> handshake after the socket is created and connected, but it fails due to >> connection timeout and leaves the socket open. It is not closed properly due >> to the exception handling in the JDK code. >> >> The change is adding a try/catch block and closing the socket in the catch >> block, and the format of the code got changed consequently. > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > update the Connection code 1) Please ignore the previous change for Connection.java 2) Refactor the method of createSocket 3) Re-write closedOpenedSocket, and remove closeSocket method. - PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1683918936
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v6]
> Please refer to JDK-8314063. > > The failure scenario is due to the setting of connection timeout. It is > either too small or not an optimal value for the system. When the client > tries to connect to the server with LDAPs protocol. It requires the handshake > after the socket is created and connected, but it fails due to connection > timeout and leaves the socket open. It is not closed properly due to the > exception handling in the JDK code. > > The change is adding a try/catch block and closing the socket in the catch > block, and the format of the code got changed consequently. Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: re-write close method - Changes: - all: https://git.openjdk.org/jdk/pull/15294/files - new: https://git.openjdk.org/jdk/pull/15294/files/3df1b6cc..ac72c3d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15294=05 - incr: https://webrevs.openjdk.org/?repo=jdk=15294=04-05 Stats: 18 lines in 1 file changed: 1 ins; 13 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15294/head:pull/15294 PR: https://git.openjdk.org/jdk/pull/15294
Re: RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection [v5]
> Please refer to JDK-8314063. > > The failure scenario is due to the setting of connection timeout. It is > either too small or not an optimal value for the system. When the client > tries to connect to the server with LDAPs protocol. It requires the handshake > after the socket is created and connected, but it fails due to connection > timeout and leaves the socket open. It is not closed properly due to the > exception handling in the JDK code. > > The change is adding a try/catch block and closing the socket in the catch > block, and the format of the code got changed consequently. Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision: update the Connection code - Changes: - all: https://git.openjdk.org/jdk/pull/15294/files - new: https://git.openjdk.org/jdk/pull/15294/files/e4898a54..3df1b6cc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15294=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15294=03-04 Stats: 58 lines in 1 file changed: 21 ins; 11 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/15294.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15294/head:pull/15294 PR: https://git.openjdk.org/jdk/pull/15294
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:02:05 GMT, Matthias Baesken wrote: > I checked the usage of sun/nio/fs/UnixConstants.java and seems we would need > to make the class public and an additional export for this like > > ``` > exports sun.nio.fs to > +java.prefs, > jdk.net; > ``` > > would this be acceptable ? No, I don't think the legacy prefs API being coupled to internals like this. Instead I think this is case where the prefs natives will need to use strerror. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683856641
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:14:35 GMT, Jaikiran Pai wrote: > Hello Matthias, I think adding some diagnostics in this class would be a good > thing. Like in the JBS issue that this PR links to, there's another JBS issue > https://bugs.openjdk.org/browse/JDK-8304938 where we run into intermittent > failures because of this exception. > > However, before doing the changes being proposed in this PR, would it be > better to do those (minimal) changes in a private build and run those tests > and see if these proposed error number or error message diagnostics do > provide the needed detail to narrow down this issue? That might provide > insight on what else (if anything) would need to be included in that > diagnostic message. > > In the meantime, I'll run your proposed change which shows the error number, > just to see if that tells us what's going on with these tests, when they fail. the enhanced output in a failing sun/tools/jhsdb/JStackStressTest.java is java.util.prefs.BackingStoreException: Couldn't get file lock. errno is 11 mode is nonshared errno 11 seems to be EAGAIN. Unfortunately even the enhanced message does not tell me if it is a failure of open or fcntl in the native layer , that would be good to know. We see this issue mostly on a particular Linux machine, probably I should ask the admins what's 'special' about this machine - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683850190
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Hello Matthias, I think adding some diagnostics in this class would be a good thing. Like in the JBS issue that this PR links to, there's another JBS issue https://bugs.openjdk.org/browse/JDK-8304938 where we run into intermittent failures because of this exception. However, before doing the changes being proposed in this PR, would it be better to do those (minimal) changes in a private build and run those tests and see if these proposed error number or error message diagnostics do provide the needed detail to narrow down this issue? That might provide insight on what else (if anything) would need to be included in that diagnostic message. In the meantime, I'll run your proposed change which shows the error number, just to see if that tells us what's going on with these tests, when they fail. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683831684
Re: RFR: 8314495: Update to use jtreg 7.3.1
On Thu, 17 Aug 2023 07:24:14 GMT, Christian Stein wrote: > Please review the change to update to using jtreg 7.3,1. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > This change set also un-problem-lists tests from > https://github.com/openjdk/jdk/commit/360f65d7b15b327e2f160c42f318945cc6548bda > > This change set also fixes: > - https://bugs.openjdk.org/browse/JDK-8313902 > - https://bugs.openjdk.org/browse/JDK-8313903 > > Testing: _tier1-tier5 pending..._ Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15323#pullrequestreview-1584457571
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding I checked the usage of sun/nio/fs/UnixConstants.java and seems we would need to make the class public and an additional export for this like exports sun.nio.fs to +java.prefs, jdk.net; would this be acceptable ? I could image there are more places in the JDK codebase where this might be interesting. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683817599
Re: RFR: 8314495: Update to use jtreg 7.3.1
On Thu, 17 Aug 2023 07:24:14 GMT, Christian Stein wrote: > Please review the change to update to using jtreg 7.3,1. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > This change set also un-problem-lists tests from > https://github.com/openjdk/jdk/commit/360f65d7b15b327e2f160c42f318945cc6548bda > > This change set also fixes: > - https://bugs.openjdk.org/browse/JDK-8313902 > - https://bugs.openjdk.org/browse/JDK-8313903 > > Testing: _tier1-tier5 pending..._ Looks good to me! Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15323#pullrequestreview-1582016262
RFR: 8314495: Update to use jtreg 7.3.1
Please review the change to update to using jtreg 7.3,1. The primary change is to the `jib-profiles.js` file, which specifies the version of jtreg to use, for those systems that rely on this file. In addition, the `requiredVersion` has been updated in the various `TEST.ROOT` files. This change set also un-problem-lists tests from https://github.com/openjdk/jdk/commit/360f65d7b15b327e2f160c42f318945cc6548bda This change set also fixes: - https://bugs.openjdk.org/browse/JDK-8313902 - https://bugs.openjdk.org/browse/JDK-8313903 Testing: _tier1-tier5 pending..._ - Commit messages: - Un-problem-list tests failing with jtreg 7.3 - 8314495: Update to use jtreg 7.3.1 Changes: https://git.openjdk.org/jdk/pull/15323/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15323=00 Issue: https://bugs.openjdk.org/browse/JDK-8314495 Stats: 12 lines in 9 files changed: 0 ins; 3 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/15323.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15323/head:pull/15323 PR: https://git.openjdk.org/jdk/pull/15323
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v12]
On Fri, 18 Aug 2023 04:41:25 GMT, sid8606 wrote: >> Implementation of "Foreign Function & Memory API" for s390x (Big Endian). > > sid8606 has updated the pull request incrementally with two additional > commits since the last revision: > > - Calculate the code size. > - Fix missing spcaces Thanks a lot for all discussions, feedback and the reviews! This was really helpful! I'm planning to integrate tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/14801#issuecomment-1683601556
Re: RFR: 8311630: [s390] Implementation of Foreign Function & Memory API (Preview) [v12]
On Fri, 18 Aug 2023 04:41:25 GMT, sid8606 wrote: >> Implementation of "Foreign Function & Memory API" for s390x (Big Endian). > > sid8606 has updated the pull request incrementally with two additional > commits since the last revision: > > - Calculate the code size. > - Fix missing spcaces Thanks for cleaning it up. LGTM. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14801#pullrequestreview-1584089670
Re: RFR: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
On Thu, 17 Aug 2023 22:35:39 GMT, Martin Buchholz wrote:
> The usual tiny improvements, with no guarantee that the intermittent test
> failure is actually fixed.
test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 41:
> 39: throws Throwable
> 40: {
> 41: int threadCount = ThreadLocalRandom.current().nextInt(2, 8);
@Martin-Buchholz I'm a bit weary about making the number of threads
random—might make it tricky to get to reproduce if the test fails?
test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 55:
> 53: Thread t = new Thread(put);
> 54: t.start();
> 55: ts.add(t);
@Martin-Buchholz Any specific reason to add the thread *after* it has been
started?
test/jdk/java/util/concurrent/SynchronousQueue/Fairness.java line 70:
> 68: fair, i, threadCount, j));
> 69: }
> 70: for (Thread t : ts) t.join();
@Martin-Buchholz Makes a lot of sense to make sure that all threads have
terminated before exiting the test
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1298155867
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1298157572
PR Review Comment: https://git.openjdk.org/jdk/pull/15337#discussion_r1298158247
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Thu, 17 Aug 2023 19:50:56 GMT, Roger Riggs wrote: >> Hi Roger, sounds like a good idea to translate it to a more informative >> string. >> Do you have an utility function in mind that does this ? > > Not trivial in java, there are platform specific native functions to get the > string for the current errno. > The function definition for `getLastErrorString` is in jni_uti.h, returning a > java String. > > If there are only a couple of errno's that occur for these cases, it might be > easier to hardwire the translation. > Otherwise, an appropriate native method call and implementation could be > added to FileSystemPreferences.c for both unix and windows. Hi Roger , seems we can get errno numbers at least from open and fcntl , see Java_java_util_prefs_FileSystemPreferences_lockFile0 https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L70 Those functions have quite a few potential errnos, from the Linux manpages at least those https://man7.org/linux/man-pages/man2/open.2.html https://man7.org/linux/man-pages/man2/fcntl.2.html EACCES EAGAIN EBADF EBUSY EDEADLK EDQUOT EEXIST EFAULT EFBIG See EOVERFLOW. EINTR EINVAL EISDIR ELOOP EMFILE ENAMETOOLONG ENFILE ENODEV ENOENT ENOLCK ENOMEM ENOSPC ENOTDIR ENXIO EOPNOTSUPP EOVERFLOW EPERM EROFS ETXTBSY EWOULDBLOCK I noticed src/java.base/unix/classes/sun/nio/fs/UnixConstants.java.template contains some of them, should I add the missing ones there and reference from there ? Btw. when looking at Java_java_util_prefs_FileSystemPreferences_lockFile0 should we better initialize `int result[2]` https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L74 There seems to be a codepath where result[1] is never written but at the end used to fill javeResult https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L117 this looks a bit problematic ? - PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1298136160
