Re: RFR: 8287766: Unnecessary Vector usage in LdapClient
On Sun, 29 May 2022 20:22:55 GMT, Andrey Turbanov wrote: > In [JDK-8273098](https://bugs.openjdk.java.net/browse/JDK-8273098) I missed > one place, where Vector could be replaced with ArrayList. > Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. Looks OK to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/8940
Re: RFR: 8287544: Replace uses of StringBuffer with StringBuilder in java.naming
On Sun, 29 May 2022 21:57:46 GMT, Andrey Turbanov wrote: > StringBuffer is a legacy synchronized class. StringBuilder is a direct > replacement to StringBuffer which generally have better performance. > There are some code that still uses StringBuffer in java.naming which could > be migrated to `StringBuilder`. Looks ok. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/8942
Re: RFR: 8287497: Use String.contains() instead of String.indexOf() in java.naming
On Sun, 29 May 2022 14:00:20 GMT, Andrey Turbanov wrote: > `String.contains` was introduced in Java 5. > Some code in java.naming still uses old approach with `String.indexOf` to > check if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. looks ok to me. - PR: https://git.openjdk.java.net/jdk/pull/8938
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes [v2]
On Sun, 20 Mar 2022 14:05:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which handles >> https://bugs.openjdk.java.net/browse/JDK-8283411? >> >> The commit here moves the temporary byte array from being a member of the >> class to a local variable within the `skip` method which is the only place >> where it is used as a temporary buffer. >> >> tier1, tier2, tier3 tests have been run successfully with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Alan's suggestion and allocate less than 512 bytes if possible. Plus > copyright year fix. Looks ok. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/7875
jdk11u build failure on Windows
Hi, I am facing the build issue with OpenJDK11(jdk11u). I am trying to build jdk11u on Windows and I am getting the below error. ./src/java.base/windows/native/libnet/net_util_md.c(792): error C2065: 'TCP_INITIAL_RTO_NO_SYN_RETRANSMISSIONS': undeclared identifier make[3]: *** [Lib-java.base.gmk:44: /cygdrive/d/source/mirrors_github_jdk11u/build/windows-x86_64-normal-server-release/support/native/java.base/libnet/net_util_md.obj] Error 1 make[3]: *** Waiting for unfinished jobs make[2]: *** [make/Main.gmk:215: java.base-libs] Error 2 ERROR: Build failed for target 'images' in configuration 'windows-x86_64-normal-server-release' (exit code 2) Stopping sjavac server ### I found that the back-port of JDK-8250521 <https://bugs.openjdk.java.net/browse/JDK-8250521> is causing this failure. My environment details are as follows. OS: Windows Server 2016 Standard, Visual Studio professional 2017 version 15.2(26430.14) When I updated my Visual Studio to 15.9.42(which contains Windows 10 SDK(10.0.17134.0)) the problem went away. My question is do we have a minimum supported Visual Studio(15.9.42) to build jdk11u(11.0.14) ?. If this is not the case then I would like to fix this build failure. Thanks, Vyom
Re: RFR: 8276042: Remove unused local variables in java.naming [v2]
On Wed, 27 Oct 2021 15:42:32 GMT, Andrey Turbanov wrote: >> 8276042: Remove unused local variables in java.naming > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8276042: Remove unused local variables in java.naming > use instanceof pattern Looks good to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/6091
Re: RFR: 8274949: Use String.contains() instead of String.indexOf() in java.base
On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov wrote: > String.contains was introduced in Java 5. > Some code in java.base still uses old approach with `String.indexOf` to check > if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. java.net changes look OK to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/5559
Re: RFR: 8274464: Remove redundant stream() call before forEach in java.* modules
On Wed, 15 Sep 2021 07:12:25 GMT, Andrey Turbanov wrote: > 8274464: Remove redundant stream() call before forEach in java.* modules Looks good to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/5520
Re: [jdk17] RFR: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
On Mon, 5 Jul 2021 11:21:39 GMT, Daniel Fuchs wrote: > Please find here a trivial fix for: > > 8269772: [macos-aarch64] test compilation failed with "SocketException: No > buffer space available" > > Running several of the websocket tests concurrently on some of the CI > machines is causing resource exhaustion, because resources are only freed > after the TIMED_WAIT delay has expired once the tests have finished. > The proposed solution is to run these tests in exclusive dir mode. Marked as reviewed by vtewari (Committer). - PR: https://git.openjdk.java.net/jdk17/pull/210
Re: RFR: 8269124: Update java.time to use switch expressions (part II)
On Tue, 22 Jun 2021 10:50:17 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review the second half of my update for the `java.time` > package to make use of switch expressions? > > This PR was split into two parts due to the large number of files affected. > > Kind regards, > > Patrick Changes looks OK to me, although i can see, in existing code couple of places we are doing conversion from long to int((int) newValue * 1000_000;) - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v3]
On Wed, 2 Jun 2021 16:37:49 GMT, Сергей Цыпанов wrote: >> There is a few JDK classes duplicating the contents of Long.hashCode() for >> hash code calculation. They should explicitly delegate to Long.hashCode(). > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8268113: Inline local vars where reasonable src/java.base/share/classes/java/util/BitSet.java line 105: > 103: * the user knows what he's doing and try harder to preserve it. > 104: */ > 105: private transient boolean sizeIsSticky = false; This change is OK, but it is not related to "8268113", do you really wants to do these changes as part of "8268113" ? - PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v9]
On Mon, 31 May 2021 14:10:50 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon 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: > > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Added missing brace > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Removed trailing whitespace > - 8267670: Remove redundant code from switch > - 8267670: Updated code to use yield > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Update java.io, java.math, and java.text to use switch expressions Marked as reviewed by vtewari (Committer). - PR: https://git.openjdk.java.net/jdk/pull/4182
Integrated: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include This pull request has now been integrated. Changeset: b823b3ef Author:Vyom Tewari URL: https://git.openjdk.java.net/jdk/commit/b823b3ef2912c4c3b8412dff6ff4e9af81c5b910 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8266797: Fix for 8266610 breaks the build on macos Reviewed-by: dholmes, jdv - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 09:50:44 GMT, Aleksey Shipilev wrote: >> this change will include the below headers files to Linux only. >> #include >> #include > > src/java.base/unix/native/libjava/io_util_md.c line 39: > >> 37: #if defined(__linux__) >> 38: #include >> 39: #include > > Does Mac OS need `sys/stat.h`, though? No, change is specific to Linux. Please see(https://github.com/openjdk/jdk/commit/69b96f9a1b4a959c6b86f41c2259d9e4d47c8ede). - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:26:30 GMT, Alan Bateman wrote: >> this change will include the below headers files to Linux only. >> #include >> #include > > src/java.base/unix/native/libjava/io_util_md.c line 256: > >> 254: return -1; >> 255: } >> 256: #if defined(__linux__) && defined(BLKGETSIZE64) > > Looks okay although would be good to fix the indentation at L256 before you > integrate. done fixed the indentation - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:09:07 GMT, Vyom Tewari wrote: >> I am not the Mac expert, but i checked on my MacOS Laptop and i did not >> found "BLKGETSIZE64" defined in sys/stat.h. >> As Alan already told that i took the code from FileDispatcherImpl.size0 and >> followed the same pattern. > > I will fix in this PR only. i will do the changes for only "io_util_md.c". updated the PR as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:59:37 GMT, Vyom Tewari wrote: >> I think Vymon took the code from FileDispatcherImpl_size0, which has been >> compiling on macOS without issues. I don't mind if we fix the issue with >> this PR or just back it out and fix it with another PR. > > I am not the Mac expert, but i checked on my MacOS Laptop and i did not found > "BLKGETSIZE64" defined in sys/stat.h. > As Alan already told that i took the code from FileDispatcherImpl.size0 and > followed the same pattern. I will fix in this PR only. i will do the changes for only "io_util_md.c". - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 10:44:10 GMT, Alan Bateman wrote: >> OK, so what you are saying is that the path that references `S_ISBLK` is >> protected by `BLKGETSIZE64` that is guaranteed to be undefined on OS X? If >> so, this is (awkwardly) fine. > > I think Vymon took the code from FileDispatcherImpl_size0, which has been > compiling on macOS without issues. I don't mind if we fix the issue with this > PR or just back it out and fix it with another PR. I am not the Mac expert, but i checked on my MacOS Laptop and i did not found "BLKGETSIZE64" defined in sys/stat.h. As Alan already told that i took the code from FileDispatcherImpl.size0 and followed the same pattern. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 11:21:34 GMT, Vyom Tewari wrote: >>> Hi Vyom, >>> >>> That fixes the include file problem but as this was obviously not tested on >>> non-Linux have you checked that the rest of the fix for 8266610 works okay >>> on non-Linux? >>> >>> Thanks, >>> David >> >> Hi David, >> >> Original fix was intended to Linux specific. Current Fix will not impact >> other platforms. >> >> There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) >> which test the new code but currently it is silently passing without running >> the test. This test requires root access. >> >> Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) >> >> Thanks, >> Vyom > >> @vyommani how are you going to test this? > > I tested locally at my Linux environment. we have a test > "java/nio/channels/FileChannel/BlockDeviceSize.java" but it requires root > privileged , i ran this as well as a root. Please have a look into > this(https://bugs.openjdk.java.net/browse/JDK-8150539). > @vyommani You can start tier1 CI build to make sure build passes with this > fix. can you please do let me know how to start tier1 CI build ? - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 06:34:36 GMT, Vyom Tewari wrote: > this change will include the below headers files to Linux only. > #include > #include somehow tests not working for me. https://github.com/vyommani/jdk/actions/runs/827989185 - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 13:01:45 GMT, Jayathirth D V wrote: > In internal CI with fix tier 1 builds are running fine. Ok i will integrate my changes now. - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 08:46:15 GMT, Vyom Tewari wrote: >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > >> Hi Vyom, >> >> That fixes the include file problem but as this was obviously not tested on >> non-Linux have you checked that the rest of the fix for 8266610 works okay >> on non-Linux? >> >> Thanks, >> David > > Hi David, > > Original fix was intended to Linux specific. Current Fix will not impact > other platforms. > > There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) > which test the new code but currently it is silently passing without running > the test. This test requires root access. > > Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) > > Thanks, > Vyom > @vyommani how are you going to test this? I tested locally at my Linux environment. we have a test "java/nio/channels/FileChannel/BlockDeviceSize.java" but it requires root privileged , i ran this as well as a root. Please have a look into this(https://bugs.openjdk.java.net/browse/JDK-8150539). - PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
On Mon, 10 May 2021 07:39:44 GMT, David Holmes wrote: > Hi Vyom, > > That fixes the include file problem but as this was obviously not tested on > non-Linux have you checked that the rest of the fix for 8266610 works okay on > non-Linux? > > Thanks, > David Hi David, Original fix was intended to Linux specific. Current Fix will not impact other platforms. There is existing test (java/nio/channels/FileChannel/BlockDeviceSize.java) which test the new code but currently it is silently passing without running the test. This test requires root access. Please see the bug( https://bugs.openjdk.java.net/browse/JDK-8150539) Thanks, Vyom - PR: https://git.openjdk.java.net/jdk/pull/3943
RFR: JDK-8266797: Fix for 8266610 breaks the build on macos
this change will include the below headers files to Linux only. #include #include - Commit messages: - fixed the indentation - added the additional check so that modified code get executed on linux only. - JDK-8266797: Fix for 8266610 breaks the build on macos Changes: https://git.openjdk.java.net/jdk/pull/3943/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3943=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266797 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3943.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3943/head:pull/3943 PR: https://git.openjdk.java.net/jdk/pull/3943
Re: RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. [v2]
On Fri, 7 May 2021 12:17:11 GMT, Vyom Tewari wrote: >> RandomAccessFile.length() method for block device return always 0 > > Vyom Tewari has updated the pull request incrementally with one additional > commit since the last revision: > > fixed assigning -1 to uint64_t I am working on it. i will provide fix ASAP. - PR: https://git.openjdk.java.net/jdk/pull/3914
Re: RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. [v2]
On Sun, 9 May 2021 06:26:13 GMT, Alan Bateman wrote: > Could required os = linux added for > test/jdk/java/nio/channels/FileChannel/BlockDeviceSize.java? As it is > decribed only run as linux. Sure, this is separate issue(https://bugs.openjdk.java.net/browse/JDK-8150539). We will fix it separately. - PR: https://git.openjdk.java.net/jdk/pull/3914
Integrated: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux.
On Fri, 7 May 2021 06:16:07 GMT, Vyom Tewari wrote: > RandomAccessFile.length() method for block device return always 0 This pull request has now been integrated. Changeset: 69b96f9a Author: Vyom Tewari URL: https://git.openjdk.java.net/jdk/commit/69b96f9a1b4a959c6b86f41c2259d9e4d47c8ede Stats: 15 lines in 1 file changed: 12 ins; 2 del; 1 mod 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. Reviewed-by: alanb, bpb - PR: https://git.openjdk.java.net/jdk/pull/3914
Re: RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. [v2]
On Sun, 9 May 2021 01:39:40 GMT, Hui Shi wrote: > Could required os = linux added for > test/jdk/java/nio/channels/FileChannel/BlockDeviceSize.java? As it is > decribed only run as linux. Sure, this is separate issue(https://bugs.openjdk.java.net/browse/JDK-8150539). We will fix it separately. - PR: https://git.openjdk.java.net/jdk/pull/3914
Re: RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. [v2]
> RandomAccessFile.length() method for block device return always 0 Vyom Tewari has updated the pull request incrementally with one additional commit since the last revision: fixed assigning -1 to uint64_t - Changes: - all: https://git.openjdk.java.net/jdk/pull/3914/files - new: https://git.openjdk.java.net/jdk/pull/3914/files/8f348ef9..72beb715 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3914=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3914=00-01 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3914.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3914/head:pull/3914 PR: https://git.openjdk.java.net/jdk/pull/3914
RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux.
RandomAccessFile.length() method for block device return always 0 - Commit messages: - JDK-8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. Changes: https://git.openjdk.java.net/jdk/pull/3914/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3914=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266610 Stats: 13 lines in 1 file changed: 10 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3914.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3914/head:pull/3914 PR: https://git.openjdk.java.net/jdk/pull/3914
Re: RFR: 8260621: (jrtfs) ThreadLocal memory leak in ImageBufferCache when using jrtfs
On Tue, 4 May 2021 09:05:38 GMT, Athijegannathan Sundararajan wrote: > Instead of BufferReference class, Map.Entry is used as pair implementation. > This avoids the metaspace leak seen via thread local. looks OK to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/3849
Re: RFR: JDK-8265496: improve null check in DeflaterOutputStream/InflaterInputStream [v2]
On Mon, 26 Apr 2021 02:36:54 GMT, Hamlin Li wrote:
>> code like below will create Deflater before null check, although it's not a
>> real mem leak, but it's better to do null check before new Deflater.
>>
>> try {
>> DeflaterOutputStream dos = new DeflaterOutputStream(null);
>> } catch (NullPointerException e) {
>> passed = true;
>> }
>> Similar issues exist in several other classes.
>
> Hamlin Li has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyrights.
Changes looks OK to me, did you change "DataOutputStreamTest.java" as well. I
can see only copyright changes ?.
-
PR: https://git.openjdk.java.net/jdk/pull/3681
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v4]
On Tue, 16 Mar 2021 02:37:24 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> An alternate approach that I thought of was to completely get rid of this >> shared cache `canonicalMap` and instead just use method level map (that gets >> initialized each time) in the `tryCanonicalize` method (thus requiring no >> locks/synchronization). I ran a JMH benchmark with the current change >> proposed in this PR and with the alternate approach of using the method >> level map. The performance number from that run showed that the approach of >> using the method level map has relatively big impact on performance (even >> when there's a cache hit in that method). So I decided to not pursue that >> approach. I'll include the benchmark code and the numbers in a comment in >> this PR, for reference. >> >> The commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Chris' suggested solution of avoiding deadlock Looks good to me. - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8263552: Use String.valueOf() for char-to-String conversion in ObjectStreamClass
On Sat, 20 Feb 2021 12:17:32 GMT, Сергей Цыпанов wrote: > This is a very simple and trivial improvement about getting rid of pointless > char wrapping into array LGTM - Marked as reviewed by vtewari (Committer). PR: https://git.openjdk.java.net/jdk/pull/2660
Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
On Wed, 10 Mar 2021 02:15:28 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed patch for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8263108? >> >> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and >> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due >> to the nature of the code involved in their static blocks. A thread dump of >> one such deadlock (reproduced using the code attached to that issue) is as >> follows: >> >> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s >> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait() [0x7a4c6000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67) >> - waiting on the Class initialization monitor for >> java.lang.constant.ConstantDescs >> at Deadlock.threadA(Deadlock.java:14) >> at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s >> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait() [0x7a5c9000] >>java.lang.Thread.State: RUNNABLE >> at >> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145) >> - waiting on the Class initialization monitor for >> java.lang.constant.DynamicConstantDesc >> at >> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239) >> at Deadlock.threadB(Deadlock.java:24) >> at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source) >> at java.lang.Thread.run(java.base@16-ea/Thread.java:831) >> >> The commit in this PR resolves that deadlock by moving the `canonicalMap` >> initialization in `DynamicConstantDesc`, from the static block, to a lazily >> initialized map, into the `tryCanonicalize` (private) method of the same >> class. That's the only method which uses this map. >> >> The implementation in `tryCanonicalize` carefully ensures that the deadlock >> isn't shifted from the static block to this method, by making sure that the >> `synchronization` on `DynamicConstantDesc` in this method happens only when >> it's time to write the state to the shared `canonicalMap`. This has an >> implication that the method local variable `canonDescs` can get initialized >> by multiple threads unnecessarily but from what I can see, that's the only >> way we can avoid this deadlock. This penalty of multiple threads >> initializing and then throwing away that map isn't too huge because that >> will happen only once when the `canonicalMap` is being initialized for the >> first time. >> >> An alternate approach that I thought of was to completely get rid of this >> shared cache `canonicalMap` and instead just use method level map (that gets >> initialized each time) in the `tryCanonicalize` method (thus requiring no >> locks/synchronization). I ran a JMH benchmark with the current change >> proposed in this PR and with the alternate approach of using the method >> level map. The performance number from that run showed that the approach of >> using the method level map has relatively big impact on performance (even >> when there's a cache hit in that method). So I decided to not pursue that >> approach. I'll include the benchmark code and the numbers in a comment in >> this PR, for reference. >> >> The commit in this PR also includes a jtreg testcase which (consistently) >> reproduces the deadlock without the fix and passes when this fix is applied. >> Extra manual testing has been done to verify that the classes of interest >> (noted in that testcase) are indeed getting loaded in those concurrent >> threads and not in the main thread. For future maintainers, there's a >> implementation note added on that testcase explaining why it cannot be moved >> into testng test. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Follow Peter Levart's review suggestion - remove volatile Your code change Looks reasonable to me. Although i am not export in this area but what i observed is the test case which was attached by original submitter passes null to DynamicConstantDesc as follows "DynamicConstantDesc.of(null)". If you pass valid argument like(ConstantDescs.BSM_INVOKE) no deadlock. - PR: https://git.openjdk.java.net/jdk/pull/2893
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
On Tue, 23 Feb 2021 12:15:08 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed copyright year in FileHandlerAccessTest.java Looks OK to me, although at my local Windows 10 box test took more than 100 iteration before it failed. - Marked as reviewed by vyomm...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8248318: Examine the use of boxing in ObjectStreamClass
On Fri, 19 Feb 2021 10:14:54 GMT, Julia Boes wrote: > This change removes some instances of superfluous boxing in > java.io.ObjectStreamClass. > Testing: tier 1-3 all clear. Looks ok to me. - Marked as reviewed by vyomm...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/2641
Re: RFR: 8253155: Minor cleanups and Javadoc fixes for LdapDnsProvider of java.naming
Hi Christoph, Changes look ok to me. On Tue, Sep 15, 2020 at 1:26 PM Christoph Langer wrote: > There are some little flaws in LdapDNSProvider and auxilliary classes, > mostly in Javadoc. > > In detail: > src/java.naming/share/classes/com/sun/jndi/ldap/DefaultLdapDnsProvider.java: > Unnecessary import > src/java.naming/share/classes/com/sun/jndi/ldap/LdapDnsProviderService.java: > typo > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProvider.java: > Whitespace > src/java.naming/share/classes/javax/naming/ldap/spi/LdapDnsProviderResult.java: > Spelling of "ldap" -> should be > capitalized > > - > > Commit messages: > - JDK-8253155 > > Changes: https://git.openjdk.java.net/jdk/pull/168/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=168=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8253155 > Stats: 21 lines in 4 files changed: 3 ins; 7 del; 11 mod > Patch: https://git.openjdk.java.net/jdk/pull/168.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/168/head:pull/168 > > PR: https://git.openjdk.java.net/jdk/pull/168 > -- Thanks, Vyom
Re: RFR 8252248: __SIGRTMAX is not declared in musl libc
+1 Vyom On Fri, Aug 28, 2020 at 8:24 PM Alexander Scherbatiy < alexander.scherba...@bell-sw.com> wrote: > On 28.08.2020 17:40, Alan Bateman wrote: > > > On 28/08/2020 15:32, Alexander Scherbatiy wrote: > >> > >> I run the java/nio/channels tests with the fix. There are five > >> failed tests that fail with and without the fix: > >> java/nio/channels/DatagramChannel/AdaptorMulticasting.java > >> java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java > >> java/nio/channels/DatagramChannel/PromiscuousIPv6.java > >> java/nio/channels/DatagramChannel/Loopback.java > >> java/nio/channels/FileChannel/FileExtensionAndMap.java > >> > >> The FileExtensionAndMap.java has clear fail message: "Error. Test > >> ignored: This test has huge disk space requirements". > > The DatagramChannel failures are probably because of firewall or > > iptables issues too. The FileExtensionAndMap test has @ignore so it > > will be skipped, maybe you are running jtreg directly and aren't use > > -ignore:quiet? >Yes, I run the jtreg tests directly without the -ignore:quiet option. > >Thanks, >Alexander. > > > > In any case, I think the changes look okay. > > > > -Alan > -- Thanks, Vyom
Re: RFR[testbug]: 8251189: com/sun/jndi/ldap/LdapDnsProviderTest.java failed due to timeout
+1 Vyom On Tue, Aug 11, 2020 at 4:39 PM Daniel Fuchs wrote: > Hi Aleksei, > > Looks good to me! > > best regards, > > -- daniel > > On 06/08/2020 13:44, Aleks Efimov wrote: > > Hi, > > > > LdapDnsProviderTest was seen failing due to the timeout caused by the > > blocked bind operation. That could happen if there is a local process > > listening on the port specified in the test URL. > > To make LdapProviderTest.ProviderTest.call fail the LDAP connect timeout > > property needs to be set to greater than 0 value. That will help > > runLocalHostTestWithRandomPort to fail with the timeout exception if the > > port is busy and try another random port. > > Also, the test output has been modified not to print stack traces for > > the expected exceptions. > > > > Webrev: http://cr.openjdk.java.net/~aefimov/8251189/00/index.html > > JBS: https://bugs.openjdk.java.net/browse/JDK-8251189 > > > > The test was executed 100+ times alongside to other LDAP tests and was > > not seen failing with the > > proposed changes. > > > > With Best Regards, > > Aleksei > > > > -- Thanks, Vyom
Re: RFR (JDK 15) 8251276: two jdeps files have extra whitespace in copyright header
LGTM Vyom On Fri, Aug 7, 2020 at 10:31 AM wrote: > Please review. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8251276 > > Webrev: http://cr.openjdk.java.net/~sundar/8251276/webrev.00/index.html > > Thanks, > > -Sundar > > -- Thanks, Vyom
Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision
Hi David, we can remove the redundant local variable(jlong result) from if block as follows. return jlong(ts.tv_sec) * MILLIUNITS + jlong(ts.tv_nsec) / NANOUNITS_PER_MILLIUNIT; Vyom On Tue, May 26, 2020 at 10:29 AM David Holmes wrote: > bug: https://bugs.openjdk.java.net/browse/JDK-8242504 > webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/ > > This work was contributed by Mark Kralj-Taylor: > > > https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html > > On the hotspot side we change the Linux implementations of > javaTimeMillis() and javaTimeSystemUTC() so that they use > clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with > our conditional use of this API I added a new guard > > os::Posix::supports_clock_gettime() > > and refined the existing supports_monotonic_clock() so that we can still > use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future > (hopefully very near future) we will simply assume these APIs always exist. > > On the core-libs side the existing test: > > test/jdk/java/time/test/java/time/TestClock_System.java > > is adjusted to track the precision in more detail. > > Finally Mark has added instantNowAsEpochNanos() to the benchmark > previously known as > > test/micro/org/openjdk/bench/java/lang/Systems.java > > which I've renamed to SystemTime.java as Mark suggested. I agree having > these three functions measured together makes sense. > > Testing: >- test/jdk/java/time/test/java/time/TestClock_System.java >- test/micro/org/openjdk/bench/java/lang/SystemTime.java >- Tiers 1-3 > > Thanks, > David > -- Thanks, Vyom
Re: RFR [LDAP]: 8062947: Fix exception message to correctly represent LDAP connection failure
LGTM as well. Vyom On Thu, May 7, 2020 at 3:01 PM Chris Yin wrote: > +1 > > Thanks, > Chris > > > On 6 May 2020, at 6:35 PM, Daniel Fuchs wrote: > > > > Hi Aleksei, > > > > Looks good to me. > > > > best regards, > > > > -- daniel > > > > On 05/05/2020 18:23, Aleks Efimov wrote: > >> "LDAP response read timed out, timeout used:-1ms.": > >> https://bugs.openjdk.java.net/browse/JDK-8062947 > >> The following fix tries to tackle this issue: > >> http://cr.openjdk.java.net/~aefimov/8062947/00 > > > > -- Thanks, Vyom
Re: [15] RFR: 8244152: Remove unnecessary hash map resize in LocaleProviderAdapters
LGTM Vyom On Thu, Apr 30, 2020 at 3:50 AM wrote: > Hello, > > Please review this small fix to the following issue: > > https://bugs.openjdk.java.net/browse/JDK-8244152 > > The proposed changeset is located at: > > https://cr.openjdk.java.net/~naoto/8244152/webrev.00/ > > The hash map used there didn't have initial capacity, even though the > exact numbers are known. > > Naoto > -- Thanks, Vyom
Re: RFR 15 8243099: Adding ADQ support to JDK
Hi Vladimir,
In LinuxSocketOptions.c we have lot's of duplicate code, can you please
reuse "socketOptionSupported" & "handleError" as follows, this we remove
lot's of duplicate code.
Thanks,
Vyom
diff -r b6b4506a7827 src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c Fri Apr 24
15:28:57 2020 +0800
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c Fri Apr 24
13:37:36 2020 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -33,6 +33,10 @@
#include "jni_util.h"
#include "jdk_net_LinuxSocketOptions.h"
+#ifndef SO_INCOMING_NAPI_ID
+#define SO_INCOMING_NAPI_ID56
+#endif
+
/*
* Class: jdk_net_LinuxSocketOptions
* Method:setQuickAck
@@ -44,15 +48,7 @@
int rv;
optval = (on ? 1 : 0);
rv = setsockopt(fd, SOL_SOCKET, TCP_QUICKACK, , sizeof
(optval));
-if (rv < 0) {
-if (errno == ENOPROTOOPT) {
-JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
-"unsupported socket option");
-} else {
-JNU_ThrowByNameWithLastError(env, "java/net/SocketException",
-"set option TCP_QUICKACK failed");
-}
-}
+handleError(env, rv, "set option TCP_QUICKACK failed");
}
/*
@@ -65,15 +61,7 @@
int on;
socklen_t sz = sizeof (on);
int rv = getsockopt(fd, SOL_SOCKET, TCP_QUICKACK, , );
-if (rv < 0) {
-if (errno == ENOPROTOOPT) {
-JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
-"unsupported socket option");
-} else {
-JNU_ThrowByNameWithLastError(env, "java/net/SocketException",
-"get option TCP_QUICKACK failed");
-}
-}
+handleError(env, rv, "get option TCP_QUICKACK failed");
return on != 0;
}
@@ -83,31 +71,18 @@
* Signature: ()Z
*/
JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_quickAckSupported0
-(JNIEnv *env, jobject unused) {
-int one = 1;
-int rv, s;
-s = socket(PF_INET, SOCK_STREAM, 0);
-if (s < 0) {
-return JNI_FALSE;
-}
-rv = setsockopt(s, SOL_SOCKET, TCP_QUICKACK, (void *) , sizeof
(one));
-if (rv != 0 && errno == ENOPROTOOPT) {
-rv = JNI_FALSE;
-} else {
-rv = JNI_TRUE;
-}
-close(s);
-return rv;
+(JNIEnv *env, jobject unused) {
+return socketOptionSupported(SOL_SOCKET, TCP_QUICKACK);
}
-static jint socketOptionSupported(jint sockopt) {
+static jint socketOptionSupported(jint level, jint optname) {
jint one = 1;
jint rv, s;
s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (s < 0) {
return 0;
}
-rv = setsockopt(s, SOL_TCP, sockopt, (void *) , sizeof (one));
+rv = setsockopt(s, level, optname, (void *) , sizeof (one));
if (rv != 0 && errno == ENOPROTOOPT) {
rv = 0;
} else {
@@ -135,8 +110,8 @@
*/
JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_keepAliveOptionsSupported0
(JNIEnv *env, jobject unused) {
-return socketOptionSupported(TCP_KEEPIDLE) &&
socketOptionSupported(TCP_KEEPCNT)
-&& socketOptionSupported(TCP_KEEPINTVL);
+return socketOptionSupported(SOL_TCP, TCP_KEEPIDLE) &&
socketOptionSupported(SOL_TCP, TCP_KEEPCNT)
+&& socketOptionSupported(SOL_TCP, TCP_KEEPINTVL);
}
/*
@@ -213,3 +188,27 @@
handleError(env, rv, "get option TCP_KEEPINTVL failed");
return optval;
}
+
+/*
+ * Class: jdk_net_LinuxSocketOptions
+ * Method:IncomingNapiIdSupported0
+ * Signature: (I)I;
+ */
+JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_IncomingNapiIdSupported0
+(JNIEnv *env, jobject unused) {
+return socketOptionSupported(SOL_SOCKET, SO_INCOMING_NAPI_ID);
+}
+
+/*
+ * Class: jdk_net_LinuxSocketOptions
+ * Method:getIncomingNapiId0
+ * Signature: (I)I;
+ */
+JNIEXPORT jint JNICALL Java_jdk_net_LinuxSocketOptions_getIncomingNapiId0
+(JNIEnv *env, jobject unused, jint fd) {
+jint optval, rv;
+socklen_t sz = sizeof (optval);
+rv = getsockopt(fd, SOL_SOCKET, SO_INCOMING_NAPI_ID, , );
+handleError(env, rv, "get option SO_INCOMING_NAPI_ID failed");
+return optval;
+}
On Fri, Apr 24, 2020 at 12:43 AM Ivanov, Vladimir A <
vladimir.a.iva...@intel.com> wrote:
> Thanks a lot to Chris and Alan for detailed comments.
> The updated version of patch available at
> http://
Re: [15] RFR: 8243138: Enhance BaseLdapServer to support starttls extended request
Hi Chris, change looks good to me. Vyom On Wed, Apr 22, 2020 at 12:59 PM Chris Yin wrote: > Hello > > Please review following change for enhancement to > com/sun/jndi/ldap/lib/BaseLdapServer.java, thanks > > Bug: https://bugs.openjdk.java.net/browse/JDK-8243138 > Webrev: http://cr.openjdk.java.net/~xyin/8243138/webrev.00/ > > There is requirement to test starttls extended op against dummy ldap > server, but unfortunately, current BaseLdapSever logic cannot handle it > correctly since the starttls request is quite special to use existed socket > for tls negotiate. This enhancement will provide the possibility to wrap > current socket connection and overwrite in-place processing input/output > stream, certainly, that will not affect existed tests which depends on > BaseLdapServer. Run existed dependency tests on 4 platforms for total 200 > times, all green. > > Thanks, > Chris -- Thanks, Vyom
Re: [15] RFR: 8242614: cleanup duplicated test ldap server in some com/sun/jndi/ldap/ tests
Hi Chris, Changes looks good to me. Thanks, Vyom On Tue, Apr 21, 2020 at 1:14 PM Chris Yin wrote: > Hello > > Please review following changes to cleanup duplicated test ldap server in > some com/sun/jndi/ldap/ tests, thanks > > Bug: https://bugs.openjdk.java.net/browse/JDK-8242614 > Webrev: http://cr.openjdk.java.net/~xyin/8242614/webrev.00/ > > This is the first part to cleanup straightforward duplicated test ldap > server which was embedded in test itself. In the past, ldap tests may copy > or implemented a dummy server in test itself to support specific test > logic, so we have many duplicated dummy server logic across tests. Now > there is already a common test ldap server skeleton available, it’s time to > cleanup those duplicated logic to reduce possible future maintenance works. > Run modified tests on 4 platforms for total 200 times, all green. > > Thanks, > Chris -- Thanks, Vyom
Re: RFR(S): 8242848: Improve performance of InflaterOutputStream.write()
Hi Volker,
Latest changes looks good to me. I notices the copyright in new test
Streams.java is "
Copyright (c) 2020, Amazon and/or its affiliates." instead is regular
Oracle copyright.
Thanks,
Vyom
On Fri, Apr 17, 2020 at 4:23 PM Volker Simonis
wrote:
> Thanks everybody for looking at this change!
>
> Please find an updated webrev (with a new test and micro-benchmark)
> and my answers to your comments below:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2020/8242848.01/
>
> On Thu, Apr 16, 2020 at 6:40 AM Vyom Tiwari wrote:
> > Thanks for doing this, i think the below code change is not required .
> >
> > Please do let me know if i am not reading it correctly.
> >
> > if (inf.finished() || (len == 0)/* no more input */) {
> >
> > If you check the native code "inf.finished() will be true only of the
> corresponding native call inflate(strm, Z_PARTIAL_FLUSH) returns
> "Z_STREAM_END".
> >
> > After your code change write may return even if finished() is not true.
>
> Yes, that's true, but that's what we must do if there's no more input
> available. Before my change this break on "len < 1" was in the "if
> (inf.needsInput())" branch.
>
> On Thu, Apr 16, 2020 at 8:22 AM Thomas Stüfe
> wrote:
> > 252 // Check the decompressor
> > 253 if (inf.finished() || (len == 0)/* no more input
> */) {
> > 254 break;
> > 255 }
> >
> > Not sure but I think this is wrong because now you bypass the followup
> handling of inf.needsDirectory.
> >
> > Inflater.inflate() returns 0 if either needsInput or needsDirectory. We
> have to ignore needsInput since we have no input anymore, but
> needsDirectory has to be handled, no?
>
> You're absolutely right Thomas. Thanks for catching this! I've moved
> the check for "needsDictionary" in front of the "finished() || len ==
> 0" check.
>
> Unfortunately there is not very good test coverage for zip with preset
> dictionaries (jtreg and submit repo passed without problems). So I
> added a new test for this use case to "
> test/jdk/java/util/zip/DeflateIn_InflateOut.java".
>
> On Thu, Apr 16, 2020 at 8:48 AM Thomas Stüfe
> wrote:
> >
> > As for increasing the buffer size, it makes sense but IMHO needs a CSR
> and a release note.
>
> I don't think so. This is an internal, implementation specific setting
> which has never been exposed or documented before so I don't see why
> we should document it now. But let's discuss this separately in the
> corresponding JBS issue (see below).
>
> On Thu, Apr 16, 2020 at 1:18 PM Claes Redestad
> wrote:
> >
> > Hi Volker,
> >
> > On 2020-04-15 19:48, Volker Simonis wrote:
> > > While doing this change, I've also realized that all the streams in
> > > java.util.zip (i.e. DeflaterInputStream, GZIPInputStream,
> > > GZIPOutputStream, InflaterInputStream, DeflaterOutputStream) use an
> > > internal byte buffer of 512 bytes by default. Looking at the benchmark
> > > attached to JDK-8242864, I think that increasing this default to a
> > > bigger size (e.g. 4096 bytes) will considerably speed up (up to 50%)
> > > read and write operations on these streams when they are created with
> > > the default buffer size. I think the value "512" is a legacy of old
> > > times when memory was more precious:) so I've opened "JDK-8242864:
> > > Increase the default, internal buffer size of the Streams in
> > > java.util.zip" to track that as as separate issue. Do you think it
> > > makes sense to increase that default value?
> >
> > Seems reasonable. 8192 seems to be the buffer size we've been converging
> > on elsewhere (see InputStream, BufferedInputStream, Files, ..). I also
>
> That seems reasonable. Alan commented on the JBS issue so we can
> continue the discussion there.
>
> > found an instance of 8096, which is either a typo or a clever
> > optimization to keep the array + array object header fit snugly within
> > 8Kb. You chose. :-)
> >
>
> I like how you try to be positive :)
>
> > >
> > > Thank you and best regards,
> > > Volker
> > >
> > > PS: do you think it makes sense to contribute the benchmark attached
> > > to JDK-8242864 to the code-tools/mh-jdk-microbenchmarks [1] project?
> > >
> > > [1]
> http://openjdk.java.net/projects/code-tools/jmh-jdk-microbenchmarks/
> >
> > I'd definitely welcome the micro as part of the patch under
> > test/micro/org/openjdk
Re: RFR(S): 8242848: Improve performance of InflaterOutputStream.write()
Hi Volker,
Thanks for doing this, i think the below code change is not required .
Please do let me know if i am not reading it correctly.
if (inf.finished() || (len == 0)/* no more input */) {
If you check the native code "inf.finished() will be true only of the
corresponding native call inflate(strm, Z_PARTIAL_FLUSH) returns
"Z_STREAM_END".
After your code change write may return even if finished() is not true.
Thanks,
Vyom
On Wed, Apr 15, 2020 at 11:19 PM Volker Simonis
wrote:
> Hi,
>
> can I please have a review for the following simple performance
> enhancement:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2020/8242848/
> https://bugs.openjdk.java.net/browse/JDK-8242864
>
> InflaterOutputStream has an internal buffer which is used for the
> inflated data. This buffer can be configured to a custom size (the
> default is 512 bytes) by using the specialized
> "InflaterOutputStream(OutputStream out, Inflater infl, int bufLen)"
> constructor. A bigger buffer, results in fewer native calls to
> "write()" on the associated OutputStream and better overall
> performance.
>
> Unfortunately "InflaterOutputStream.write(byte[] b, int off, int len)"
> unnecessarily chunks its own byte input buffer "b" into pieces of
> maximum 512 bytes before feeding them to the underlying Inflater. This
> unnecessarily increases the number of native calls to
> Inflater.inflate() and limits the benefit of the configurable internal
> buffer to a size of ~(512 * compression-ratio) bytes.
>
> Instead, we could easily pass the complete input buffer "b" as input
> to the underlying Inflater. This simplifies the code and results in up
> to ~25% performance improvements for bigger internal buffers (see the
> JMH benchmark attached to the bug).
>
> Regarding the implementation, I initially wanted to completely remove
> the "for (;;)" loop after removing the chunking of the input buffer.
> But I think it is still necessary, because an InflaterOutputStream can
> be created with a custom Inflater which already has some input data.
> In that case, the Inflater will first consume its initial input data
> in the first "for" loop iteration while "write()"s input buffer "b"
> will only be consumed in the second "for" loop iteration.
>
> While doing this change, I've also realized that all the streams in
> java.util.zip (i.e. DeflaterInputStream, GZIPInputStream,
> GZIPOutputStream, InflaterInputStream, DeflaterOutputStream) use an
> internal byte buffer of 512 bytes by default. Looking at the benchmark
> attached to JDK-8242864, I think that increasing this default to a
> bigger size (e.g. 4096 bytes) will considerably speed up (up to 50%)
> read and write operations on these streams when they are created with
> the default buffer size. I think the value "512" is a legacy of old
> times when memory was more precious :) so I've opened "JDK-8242864:
> Increase the default, internal buffer size of the Streams in
> java.util.zip" to track that as as separate issue. Do you think it
> makes sense to increase that default value?
>
> Thank you and best regards,
> Volker
>
> PS: do you think it makes sense to contribute the benchmark attached
> to JDK-8242864 to the code-tools/mh-jdk-microbenchmarks [1] project?
>
> [1] http://openjdk.java.net/projects/code-tools/jmh-jdk-microbenchmarks/
>
--
Thanks,
Vyom
Re: [15] RFR: 8214694: cleanup rawtypes warnings in open jndi tests
Hi Chris, Latest changes look good to me. I can see that there are couple of unused imports in files(DeadServerTimeoutSSLTest.java) but unused imports are separate issue. Thanks, Vyom On Fri, Mar 27, 2020 at 2:48 PM Chris Yin wrote: > Hi, Vyom > > On 27 Mar 2020, at 12:08 PM, Vyom Tiwari wrote: > > Hi Chris, > > I have one question to you, is there is any specific reason for using > wildcard(?) ?. > > > Thank you for reviewing and comments. I just replaced most of the > wildcard(?) with specified type as precise as they could be in latest > webrev.01, the rest of them may fall into below scenarios. > > 1. API return value or parameter with wildcard(?), such as Hashtable > in test/jdk/com/sun/jndi/dns/EnvTests/AddInherited.java > 2. Cannot find the precise type from code, such as ScheduledFuture > in test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java > > In your change we can avoid "?" at most of the places. Please see the > below methods signatures. > > ### > public NamingEnumeration listBindings(Name name) throws > NamingException; > public NamingEnumeration list(Name name) throws > NamingException; > public NamingEnumerationsearch(Name name, Attributes > matchingAttributes, > > > String[] attributesToReturn) throws NamingException; > # > > > Thank you for the detailed signatures info, yes, now all fixed in the > latest webrev http://cr.openjdk.java.net/~xyin/8214694/webrev.01/ > > Regards, > Chris > > > thanks, > Vyom > > On Wed, Mar 25, 2020 at 1:28 PM Chris Yin wrote: > >> Hello >> >> Please review following simple changes to cleanup raw types warning for >> open jndi tests (under test/jdk/com/sun/jndi and test/jdk/javax/naming), >> thanks >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8214694 >> Webrev: http://cr.openjdk.java.net/~xyin/8214694/webrev.00/ >> >> >> The changes should be straightforward, only fix raw types warnings, no >> test logic change, no code optimization or cleanup. Minor change to each >> test file, just a little surprised about the affected tests count, hope >> this covers all. Run related jndi tests on 4 platforms for total 200 times, >> all passed. >> >> Thanks, >> Chris > > > > -- > Thanks, > Vyom > > > -- Thanks, Vyom
Re: [15] RFR: 8214694: cleanup rawtypes warnings in open jndi tests
Hi Chris, I have one question to you, is there is any specific reason for using wildcard(?) ?. In your change we can avoid "?" at most of the places. Please see the below methods signatures. ### public NamingEnumeration listBindings(Name name) throws NamingException; public NamingEnumeration list(Name name) throws NamingException; public NamingEnumerationsearch(Name name, Attributes matchingAttributes, String[] attributesToReturn) throws NamingException; # thanks, Vyom On Wed, Mar 25, 2020 at 1:28 PM Chris Yin wrote: > Hello > > Please review following simple changes to cleanup raw types warning for > open jndi tests (under test/jdk/com/sun/jndi and test/jdk/javax/naming), > thanks > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214694 > Webrev: http://cr.openjdk.java.net/~xyin/8214694/webrev.00/ > > > The changes should be straightforward, only fix raw types warnings, no > test logic change, no code optimization or cleanup. Minor change to each > test file, just a little surprised about the affected tests count, hope > this covers all. Run related jndi tests on 4 platforms for total 200 times, > all passed. > > Thanks, > Chris -- Thanks, Vyom
Re: [15] RFR: 8202117: com/sun/jndi/ldap/RemoveNamingListenerTest.java fails intermittently: Connection reset
looks good to me.
Vyom
On Mon, Mar 16, 2020 at 12:59 PM Chris Yin wrote:
> Thanks for reviewing, Daniel, Vyom.
>
>
> Hi, Daniel
>
> I modified the test as you suggested to cover the potential issue with
> URIBuilder, many thanks. Updated webrev as below:
>
> http://cr.openjdk.java.net/~xyin/8202117/webrev.01/
>
> Regards,
> Chris
>
>
> > On 13 Mar 2020, at 7:51 PM, Daniel Fuchs
> wrote:
> >
> > Hi Chris,
> >
> > This looks fine to me too. Thanks for taking care of this issue.
> >
> > A potential issue I see is that the test might fail if
> > "localhost" does not resolve to the loopback address - but you
> > would likely get a "Connection refused" in that case.
> >
> > One possibility to get that out of the way could be to use
> > the URIBuilder:
> >
> > * @library lib/ /test/lib
> >
> > ...
> >
> > URI providerURI = URIBuilder.newBuilder()
> >.scheme("ldap")
> >.loopback()
> >.port(server.getLocalPort())
> >.path("/o=example")
> >.build();
> > ...
> >
> > 59 env.put(Context.PROVIDER_URL, providerURI.toString());
> >
> > best regards,
> >
> > -- daniel
> >
> >
> > On 13/03/2020 08:28, Chris Yin wrote:
> >> Hello,
> >> Please review following changes to try to fix intermittent failure of
> test com/sun/jndi/ldap/RemoveNamingListenerTest.java, thanks
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8202117
> >> Webrev: http://cr.openjdk.java.net/~xyin/8202117/webrev.00/
> >> According to failure logs, test already run done and give a pass
> message, but test framework caught “java.lang.RuntimeException:
> java.net.SocketException: Connection reset” from other thread during test
> end, go through the test code, LDAPServerHandler thread may throw such
> exception in specific case. This change will replace test itself
> implemented TestLDAPServer/LDAPServerHandler with customized BaseLdapServer
> to fix the corner. I had run the changed test on 4 platforms for total 600
> times, no failure observed.
> >> Thanks,
> >> Chris
> >
>
>
--
Thanks,
Vyom
Re: [15] RFR: 8202117: com/sun/jndi/ldap/RemoveNamingListenerTest.java fails intermittently: Connection reset
Hi Chris, Thanks for taking care of this issue, changes looks OK to me. I am happy to see that we are removing the lot's of duplicate "DummyServer" from individual test cases and using the "BaseLdapServer" test-server instead. Thanks, Vyom On Fri, Mar 13, 2020 at 1:59 PM Chris Yin wrote: > Hello, > > Please review following changes to try to fix intermittent failure of test > com/sun/jndi/ldap/RemoveNamingListenerTest.java, thanks > > Bug: https://bugs.openjdk.java.net/browse/JDK-8202117 > Webrev: http://cr.openjdk.java.net/~xyin/8202117/webrev.00/ > > > According to failure logs, test already run done and give a pass message, > but test framework caught “java.lang.RuntimeException: > java.net.SocketException: Connection reset” from other thread during test > end, go through the test code, LDAPServerHandler thread may throw such > exception in specific case. This change will replace test itself > implemented TestLDAPServer/LDAPServerHandler with customized BaseLdapServer > to fix the corner. I had run the changed test on 4 platforms for total 600 > times, no failure observed. > > Thanks, > Chris -- Thanks, Vyom
Re: RFR 8240524: Removed warnings from test classes
Hi Vipin, Test code changes looks good to me as well except copyright changes, which will be fix at the time of pushing the code by Christoph. Thanks, Vyom On Fri, Mar 6, 2020 at 12:06 PM Langer, Christoph wrote: > Hi Vipin, > > this all looks correct to me. > > When changing the copyright headers, you have to keep the first (initial) > year and only update the second year. If this doesn’t exist, you’ll have to > add it, e.g. > Copyright (c) 1999, Oracle -> Copyright (c) 1999, 2020, Oracle > > I can fix that for you though, when sponsoring, no need for new webrev. > > Can I have a second review, please? > > Thanks > Christoph > > > > From: Vipin Sharma > Sent: Donnerstag, 5. März 2020 18:27 > To: Langer, Christoph ; > core-libs-dev@openjdk.java.net > Subject: RFR 8240524: Removed warnings from test classes > > Hi All, > > Please review patch to remove warnings from test classes. > > Bug : https://bugs.openjdk.java.net/browse/JDK-8240524 > Webrev : http://cr.openjdk.java.net/~clanger/webrevs/8240524.0/ > > > Change description: > > Class: test/jdk/java/lang/Boolean/GetBoolean.java > Fixed following warning: > 1. "Exception 'java.lang.Exception' is never thrown in the method” > > Class: test/jdk/java/lang/Boolean/MakeBooleanComparable.java > Fixed following warnings: > 1. Explicit type argument Boolean can be replaced with <> > 2. C-style array declaration of parameter 'args' > > Class: test/jdk/java/lang/Boolean/ParseBoolean.java > Fixed following warning: > 1. Exception 'java.lang.Exception' is never thrown in the method > > > Regards, > Vipin > -- Thanks, Vyom
RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read
Hi All,
Please find the below fix which resolves the issue(
https://bugs.openjdk.java.net/browse/JDK-8238579).
"HttpURLConnection.writeRequests()" retry in case of any write failure,
during retry it creates new HttpsClient without connectTimeout &
readTimeout. Below fix sets the connect & read timeout.
Thanks,
Vyom
diff -r 7e6165c9c606
src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
---
a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Thu Feb 13 17:14:45 2020 -0800
+++
b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Fri Feb 14 10:11:06 2020 +0530
@@ -87,10 +87,15 @@
*/
public void setNewClient (URL url, boolean useCache)
throws IOException {
+int readTimeout = getReadTimeout();
http = HttpsClient.New (getSSLSocketFactory(),
url,
getHostnameVerifier(),
-useCache, this);
+ null,
+ -1,
+useCache,
+ getConnectTimeout(), this);
+ http.setReadTimeout(readTimeout);
((HttpsClient)http).afterConnect();
}
@@ -132,10 +137,14 @@
boolean useCache) throws IOException {
if (connected)
return;
-http = HttpsClient.New (getSSLSocketFactory(),
-url,
-getHostnameVerifier(),
-proxyHost, proxyPort, useCache, this);
+int readTimeout = getReadTimeout();
+http = HttpsClient.New(getSSLSocketFactory(),
+url,
+getHostnameVerifier(),
+proxyHost, proxyPort,
+useCache,
+getConnectTimeout(), this);
+http.setReadTimeout(readTimeout);
connected = true;
}
Re: [teststabilization] RFR: 8141685: com/sun/jndi/ldap/InvalidLdapFilters.java initializes context failed
Hi Daniel, Ok, thanks for the update. Vyom On Sun, Dec 8, 2019 at 4:19 PM Daniel Fuchs wrote: > Hi Vyom, > > On 07/12/2019 03:36, Vyom Tiwari wrote: > > Changes looks OK to me, one minor bit you need to update @bug tag > > before pushing. > > We don't update @bug for test bugs (when the fix is in the > the test itself). > > best regards, > > -- daniel > -- Thanks, Vyom
Re: [teststabilization] RFR: 8141685: com/sun/jndi/ldap/InvalidLdapFilters.java initializes context failed
Hi Aleks, Changes looks OK to me, one minor bit you need to update @bug tag before pushing. Thanks, Vyom On Fri, Dec 6, 2019 at 11:41 PM Daniel Fuchs wrote: > Looks good to me Aleksei! > > best regards, > > -- daniel > > On 06/12/2019 17:33, Aleks Efimov wrote: > > Hi, > > > > InvalidLdapFilters test has been seen failing intermittently with > > timeouts. The cause of the observed failures could be the binding of > > test server to the wildcard address. > > Proposed fix binds the test server to the loopback address and uses the > > test library to construct LDAP provider URL. > > The modified test has been executed ~100 times alongside to other > > networking and JNDI tests: No failures have been observed. > > > > Webrev: http://cr.openjdk.java.net/~aefimov/8141685/00 > > JBS: https://bugs.openjdk.java.net/browse/JDK-8141685 > > > > With Best Regards, > > Aleksei > > > > -- Thanks, Vyom
Re: RFR: 8234964: failure_handler: gather more environment information on Windows, Solaris and Linux
Hi Julia, changes looks good to me. Thanks, Vyom On Mon, Dec 2, 2019 at 4:24 PM Julia Boes wrote: > Hi, > > > > to make it more consistent w/ other tools, I'd move all ifconfig (incl. > one on macOS) to 'net' category, i.e. rename them to net.ifconfig, this > will require also moving all netstat.* on macOS and solaris to 'net' as > well. I don't insist on it, though. > > Good point, I made those changes. > > > > Changes looks OK to me although i have one question, in case of > > Solaris you use explicit path to ifconfig(ifconfig.app=/sbin/ifconfig). > > Does Solaris by default doesn't set the /sbin folder to user's 'PATH' > > environment variable ? > > That's right, /sbin is not on the PATH on the Solaris machines of the > test farm, which can be confirmed under the 'env' section of the > environment information output. > > > Updated webrev: > http://cr.openjdk.java.net/~jboes/webrevs/8234964/webrev.01/ > > > Regards, > > Julia > > > -- Thanks, Vyom
Re: RFR: 8234964: failure_handler: gather more environment information on Windows, Solaris and Linux
Hi Julia, Changes looks OK to me although i have one question, in case of Solaris you use explicit path to ifconfig(ifconfig.app=/sbin/ifconfig). Does Solaris by default doesn't set the /sbin folder to user's 'PATH' environment variable ?. Thanks, Vyom On Fri, Nov 29, 2019 at 12:18 AM Igor Ignatyev wrote: > Hi Julia, > > looks good to me. > > to make it more consistent w/ other tools, I'd move all ifconfig (incl. > one on macOS) to 'net' category, i.e. rename them to net.ifconfig, this > will require also moving all netstat.* on macOS and solaris to 'net' as > well. I don't insist on it, though. > > -- Igor > > > On Nov 28, 2019, at 10:15 AM, Julia Boes wrote: > > > > Hi, > > > > In the case of test failure, the environment information of 'ifconfig > -a' is already gathered on macOS systems. > > > > The following enhancement allows the same information to be gathered on > Linux, Solaris and Windows systems (in the latter case 'ipconfig /all'). > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8234964 > > > > Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8234964/webrev.00/ > > > > Regards, > > > > Julia > > > > -- Thanks, Vyom
Re: RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory
Hi Joe, Fix looks OK to me , but i am not able to understand how come "NamespaceMappings" instance can increase memory uses from (20MB to 140MB ). Current scope of "ns" is "case Node.ELEMENT_NODE:" block and "NamespaceMapping" seems to be very lightweight class. Thanks, Vyom On Fri, Nov 8, 2019 at 12:33 AM Joe Wang wrote: > Please review a quick fix that reduces unnecessary object allocations. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8233686 > webrev: http://cr.openjdk.java.net/~joehw/jdk14/8233686/webrev/ > > Thanks, > Joe > > -- Thanks, Vyom
Re: 8229022: BufferedReader performance can be improved by using StringBuilder
Hi Brian,
Looks good to me.
thanks,
Vyom
On Wed, Oct 2, 2019 at 5:44 AM Brian Burkhalter
wrote:
> While the performance improvement that I measured for this proposed change
> [1] using JMH was only in the 2% to 8% range as opposed to the 13% claimed
> in the issue description, given the simplicity of the change [2] it is
> probably worth doing. All use of the StringBuilder is within a synchronized
> block within a single package-scope method, so there should be no problem
> with access by multiple threads.
>
> Thanks,
>
> Brian
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8229022
> [2] diff
>
> --- a/src/java.base/share/classes/java/io/BufferedReader.java
> +++ b/src/java.base/share/classes/java/io/BufferedReader.java
> @@ -314,7 +314,7 @@
> * @throws IOException If an I/O error occurs
> */
> String readLine(boolean ignoreLF, boolean[] term) throws IOException {
> -StringBuffer s = null;
> +StringBuilder s = null;
> int startChar;
>
> synchronized (lock) {
> @@ -372,7 +372,7 @@
> }
>
> if (s == null)
> -s = new StringBuffer(defaultExpectedLineLength);
> +s = new StringBuilder(defaultExpectedLineLength);
> s.append(cb, startChar, i - startChar);
> }
> }
>
>
--
Thanks,
Vyom
Re: RFR (14) [testbug]: runs zero test, 8230002 and 8230010
Hi Frank, Changes looks good to me. Thanks, Vyom On Tue, Aug 27, 2019 at 2:37 PM Frank Yuan wrote: > Hi all > > > > We found 2 jaxp tests, which didn't run indeed. > > > > Bugs: > > https://bugs.openjdk.java.net/browse/JDK-8230002 > > Annotation @Test was missed in TestNG test. > > > > https://bugs.openjdk.java.net/browse/JDK-8230010 > > The old test was left during it was converted to TestNG test. > > > > Webrev: > > http://cr.openjdk.java.net/~fyuan/8230002_8230010/webrev.00/ > > > > Thanks > > Frank > > -- Thanks, Vyom
Re: [PATCH] JDK-8228580 DnsClient TCP socket timeout
Hi Milan, Your test need the corresponding "TcpTimeout.dns" file to run successfully, I believe you forgot to add with your patch. please check the existing tests in the same folder if you need any additional information. Thanks, Vyom On Wed, Aug 21, 2019 at 10:48 PM Milan Mimica wrote: > Hi Pavel > > Updated the patch with the jtreg test. > The test hangs when the fix is not applied. I don't know why > main/timeout=20 does not work for me. > > On Tue, 20 Aug 2019 at 00:08, Pavel Rappo wrote: > > > > Thanks for doing that. I've only skimmed through the patch and I’d > recommend that no matter if the changes are adequate or not there should be > a test [1]: > > > > "...A unit test, written for the jtreg test harness, that validates your > change. The test should fail when run against a build without the change > and succeed when run against a build with the change. Unit tests aren't > always practical, especially for changes such as performance improvements > or fixes to bugs that are highly platform-dependent, but if practical then > a unit test is required." > > > > Please consider adding it to the patch while the changes are being > (preliminary) reviewed. > > > > -Pavel > > > > > --- > > [1] https://openjdk.java.net/contribute/ > > > > > On 19 Aug 2019, at 17:20, Milan Mimica wrote: > > > > > > Hello list > > > > > > Please find the attached patch that fixes JDK-8228580. > > > > > > It works the similar way UDP timeout does: calculate the initial > > > timeout from retry attempt, and account for duration of every blocking > > > call on the TCP socket. > > > > > > I am listed as Author[1] on "jdk" project. > > > > > > [1] https://openjdk.java.net/census#mmimica > > > > > > > > > -- > > > Milan Mimica > > > > > > > > -- > Milan Mimica > -- Thanks, Vyom
Re: [RFR] 8214440: ldap over a TLS connection negotiate failed with "javax.net.ssl.SSLPeerUnverifiedException: hostname of the server '' does not match the hostname in the server's certificate"
Hi Rob,
Thanks for fixing this issue, please use hostname.isEmpty(), instead of
"".equal(hostname). I have a question to you, why we are converting null
to empty string("") in LdapDnsProvider ?.
If you see the java doc it tells that domainname can be null
/**
* Construct an LdapDnsProviderResult consisting of a resolved
domain name
* and the ldap server endpoints that serve the domain.
*
* @param domainName the resolved domain name; can be null.
My personal suggestion is not to replace null to empty string("") in
"LdapDnsProviderResult" either throw some exception or just pass
whatever you got in LdapDnsProviderResult constructor.
Thanks,
Vyom
On 08/01/19 10:33 PM, Rob McKenna wrote:
Hi folks,
I'd like to fix this test failure caused by 8160768.
The problem is that the LdapDnsProviderResult sets the hostname to the
empty String and gets passed to StartTlsResponseImpl.verify.
Unfortunately StartTlsResponseImpl.verify only expects null values.
Since null and the empty String are functionally equivalent I've added a
check to StartTlsResponseImpl.verify to take the empty String into
account.
http://cr.openjdk.java.net/~robm/8214440/webrev.01/
This was caught by an existing test which I managed to miss in my
testing incantation.
-Rob
Re: [12] RFR JDK-8214241: Problem list com/sun/jndi/ldap/LdapTimeoutTest.java on all platforms
looks OK to me. Vyom On 23/11/18 9:28 AM, Amy Lu wrote: Please review the patch to problem list com/sun/jndi/ldap/LdapTimeoutTest.java This test fails frequently, originally observed on linux-x64 but recently also on other platforms, and should be problem listed before JDK-8151678 fixed. bug: https://bugs.openjdk.java.net/browse/JDK-8214241 webrev: http://cr.openjdk.java.net/~amlu/8214241/webrev.00/ Thanks, Amy --- old/test/jdk/ProblemList.txt 2018-11-23 11:53:27.0 +0800 +++ new/test/jdk/ProblemList.txt 2018-11-23 11:53:26.0 +0800 @@ -876,7 +876,7 @@ com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java 8169942 linux-i586,macosx-all,windows-x64 -com/sun/jndi/ldap/LdapTimeoutTest.java 8151678 linux-all +com/sun/jndi/ldap/LdapTimeoutTest.java 8151678 generic-all com/sun/jndi/dns/ConfigTests/PortUnreachable.java 7164518 macosx-all
Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider
Hi Rob, Overall your code looks OK to me, In LdapDnsProviderService.java please make 'service' as volatile otherwise 'getInstance()' may not work as expected. I can see you implemented DCL but it may not work if 'service' is not declare volatile. If you like, you can us the initialization 'On Demand Holder idiom' to implement the lazy initialization. Thanks, Vyom On 26/10/18 8:31 PM, Rob McKenna wrote: Yes, thanks a lot Alan. Vyom and Martin both had some very helpful feedback so I'm hoping to hear from both! -Rob On 26/10/18 15:34, Alan Bateman wrote: On 25/10/2018 17:34, Rob McKenna wrote: This recently received CSR approval, so it seems like a good time to pick the codereview up again: http://cr.openjdk.java.net/~robm/8160768/webrev.08/ I went through a number of iterations with Rob on the API/javadoc so I think the API parts in webrev.08 are good. I have not had time to go through the implementation and test changes so hopefully that someone else has cycles to help on that. -Alan
Re: RFR[12] JDK-8211107: LDAPS communication failure with jdk 1.8.0_181
Hi Prasad, Change looks good to me, can you please put some comment why we are not performing "ssl handshake" if connectTime <= 0. This will help future maintainer of this code. Thanks, Vyom On Tuesday 02 October 2018 06:39 AM, Prasadrao Koppula wrote: Hi, Could you please review this patch. This patch fixes the regression caused by LDAP fixes in JDK11, JDK8u181, JDK7u191 Webrev: http://cr.openjdk.java.net/~pkoppula/8211107/webrev.01/ Issue: https://bugs.openjdk.java.net/browse/JDK-8211107 Thanks, Prasad.K
Re: [12] RFR 8210339: Add 10 JNDI tests to com/sun/jndi/dns/FedTests/
Hi Chris, tests looks good to me. Thanks, On Tuesday 04 September 2018 12:00 PM, Chris Yin wrote: Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/FedTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8210339 webrev: http://cr.openjdk.java.net/~xyin/8210339/webrev.00/ Regards, Chris
Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection
Hi Chris,Daniel, Please find the latest patch( http://cr.openjdk.java.net/~vtewari/8205330/webrev0.1/index.html ). I did the additional cleanup(removed redundant null check) as you suggested. Thanks, Vyom On Monday 10 September 2018 09:03 PM, Daniel Fuchs wrote: On 10/09/2018 15:53, vyom tewari wrote: Yes , this will definitely solve this NPE issue but we may hit NPE other places as well because we are setting 'LdapClient.com' to null asynchronously. I agree with Vyom here. Other solutions that have been investigated - such as only setting the connection to null when its ref count reaches zero now seem to complex (read: too risky because over the complexity threshold) to me. Maybe the best thing to do here is to stick to Vyom original's idea, but declare conn final and do all the necessary (small) cleanup that goes with it? Possibly also add a comment saying that CommunicationException will be thrown if some other thread uses the LdapClient after forceClose has been closed... best regards, -- daniel
Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection
On Monday 10 September 2018 05:30 PM, Chris Hegarty wrote:
Vyom,
The NPE is originating from the finally block of the LdapClient
`authenticate` method. In the finally block there is an attempt
to restore the "read" timeout, since it was changed earlier in
`authenticate`, to reflect the connect timeout value, since the
subsequent operation(s) are considered part of the connect phase.
An unsolicited message may close the connection during
authenticate, which is does and LdapClient.ldapBind throws
"javax.naming.CommunicationException: Request: 1 cancelled". The
finally block is then executed and the previous exception is
effectively throw away when the NPE is encountered.
If we agree that such behaviour is reasonable ( and I think it
most likely is ), then the finally block needs to be more
defensive - check that conn is not null. It makes no sense to
attempt to reset the read timeout if conn is null.
With the following change, then the CommunicationException will
be thrown from `authenticate`. I think this is the behaviour we
want, no?
Yes , this will definitely solve this NPE issue but we may hit NPE other
places as well because we are setting 'LdapClient.com' to null
asynchronously.
Vyom
--- a/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java
+++ b/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java
@@ -297,7 +297,8 @@
conn.setV3(isLdapv3);
return res;
} finally {
- conn.readTimeout = readTimeout;
+ if (conn != null)
+ conn.readTimeout = readTimeout;
}
}
-Chris.
On 07/09/18 17:47, Daniel Fuchs wrote:
Hi Vyom,
Please see inline... I've elided some parts...
On 07/09/2018 10:11, vyom tewari wrote:
On Tuesday 04 September 2018 08:13 PM, Daniel Fuchs wrote:
So what we see here is actually a race between an open
connection being retrieved from the pool, and the server
deciding to close that connection.
The connection/client is retrieved from the pool, then
unfortunately closed by the server after having been selected
for the next operation.
I checked the code and i did not found any problem. Both "get" and
"removePooledConnection" are "synchronized" so there should not be
any problem(concurrency issue) here.
I'm referring to this pattern (in forceClose):
conn.cleanup(null, false);
conn = null;
if (cleanPool) {
pcb.removePooledConnection(this);
}
The connection is cleaned up, then nulled, then only removed
from the pool. I'm questioning whether the first thing to do would
be to remove the connection from the pool, to reduce the time window
in which the client could be retrieved from the pool by another
thread while forceClose is in process.
I am not sure what side effect this could have - as I haven't
studied the code that retrieves the client from the pool,
but I'm wondering whether that's worth exploring.
As you said server is closing the same connection which is retrieved
from pool which is correct but this should not throw unintentional
NPE.
Agreed.
In LdapClient, we set the "Ldapclient.conn" to explicitly null
asynchronously after we remove the connection from pool and which is
creating the NPE.
LdapClient does not set `Ldapclient.conn` to null if connection is
not pooled.
Really? That's not what I see... Am I missing something?
>> [...]
If so will leaving the 'conn' field assigned ensure that
the retry happens, or will new InitialDirContext() fail
with some other exception because the connection has
been closed? Or does the code simply assume that asynchronous
closing of the connection by the server can only happen while
it's sitting idle in the pool?
I don't know if retrying is feasible in 'IntialDirContext' but if
we leave 'LdapClient.conn' assigned then we will get
"javax.naming.CommunicationException" which tells that, underline
connection is closed. I am not 100% sure if this is right approach
but if we set 'LdapClient.conn' to null asynchronously then we will
hit NPE.
OK - that's good to know. I agree that CommunicationException is
better than NPE. If so then not nulling the connection
is at least an improvement.
But this would deserve a comment in the code, I think.
More below...
I wonder if some other mechanism could be used to reduce
and hopefully eliminate the time window in which the race
could appear. For instance taking the connection out of
the pool before closing it, instead of closing it before
taking it out of the pool, etc...
Changing order will not help, i think closing the same connection
is not a problem but setting `LdapClient.conn' to null
asynchronously is causing NPE.
Yes - I agree that setting conn to null is problematic.
I wonder why it's set to null. Maybe in an attempt to
make it eligible for GC earlier?
If that's not the issue, then I would suggest maki
Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection
Hi Daniel, Thanks for detailed review and comment. Please find my answers inline. Thanks, Vyom On Tuesday 04 September 2018 08:13 PM, Daniel Fuchs wrote: Hi Vyom, IIUC, the issue only happens when connections (clients?) to the server are pooled. I assume the server may decide to close what it thinks is an idle connection at any time. correct So what we see here is actually a race between an open connection being retrieved from the pool, and the server deciding to close that connection. The connection/client is retrieved from the pool, then unfortunately closed by the server after having been selected for the next operation. I checked the code and i did not found any problem. Both "get" and "removePooledConnection" are "synchronized" so there should not be any problem(concurrency issue) here. As you said server is closing the same connection which is retrieved from pool which is correct but this should not throw unintentional NPE. In LdapClient, we set the "Ldapclient.conn" to explicitly null asynchronously after we remove the connection from pool and which is creating the NPE. LdapClient does not set `Ldapclient.conn` to null if connection is not pooled. The question for me would be "what is the expected behavior if the server decides to close an idle connection?" I would expect that new InitialDirContext() should have some way of dealing with that with retrying? If so will leaving the 'conn' field assigned ensure that the retry happens, or will new InitialDirContext() fail with some other exception because the connection has been closed? Or does the code simply assume that asynchronous closing of the connection by the server can only happen while it's sitting idle in the pool? I don't know if retrying is feasible in 'IntialDirContext' but if we leave 'LdapClient.conn' assigned then we will get "javax.naming.CommunicationException" which tells that, underline connection is closed. I am not 100% sure if this is right approach but if we set 'LdapClient.conn' to null asynchronously then we will hit NPE. I wonder if some other mechanism could be used to reduce and hopefully eliminate the time window in which the race could appear. For instance taking the connection out of the pool before closing it, instead of closing it before taking it out of the pool, etc... Changing order will not help, i think closing the same connection is not a problem but setting `LdapClient.conn' to null asynchronously is causing NPE. Please do let me know if i am missing something. best regards, -- daniel On 04/09/2018 15:04, vyom tewari wrote: On Friday 24 August 2018 08:52 PM, Chris Hegarty wrote: Hi Vyom, On 24/08/18 11:35, vyom tewari wrote: Hi All, Please review this simple fix below webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html bugid: https://bugs.openjdk.java.net/browse/JDK-8205330 This fix will resolve the race in LdapClient where we are explicitly making "null" to LdapClient.conn. Sorry, I don't know this code all that well, but I think that more explanation will be needed before this code can be reviewed. Hi Chris, let me try to explain issue little bit. The issue is a if ldap connection has already been established and then the LDAP directory server sends an (unsolicited) "Notice of Disconnection", the client's processing of this LDAP message can race with an application thread calling new InitialDirContext() to authenticate user credentials (i.e.bind) and throw NPE. I did some analysis and found out that when server send an (unsolicited) "Notice of Disconnection", LdapClient.forceClose() will be called in LdapClient.processUnsolicited() which is called asynchronously by the reader thread in Connection, this means 'LdapClient.conn' may set to null anytime after it received "Notice of Disconnection". The LdapClient and the Connection seem to be loosely coupled. I think part of this is to allow the LdapClient to be GC'ed and finalized separately to the Connection ( that can be reused ). Not setting `conn` to null could have a negative impact on this loose coupling. I also note that the synchronization is implemented poorly in the LdapClient, `conn` is operated on both from within synchronized blocks and from unsynchronized blocks ( which I think is the reason you see the unexpected null ). I agree, not setting 'conn' to null will definitely have some impact. I check the LdapClient and it looks to me it is intentional(i may be wrong) that 'conn' is operated on both from within synchronize blocks and from unsynchronize block. LdapClient, java doc says that connection(conn) take care of it's own sync. ## access from outside LdapClient must sync; * conn - no sync; Connection takes care of its own sync * unsolicited - sync Vector; multiple operations sync'ed
Re: RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection
On Friday 24 August 2018 08:52 PM, Chris Hegarty wrote: Hi Vyom, On 24/08/18 11:35, vyom tewari wrote: Hi All, Please review this simple fix below webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html bugid: https://bugs.openjdk.java.net/browse/JDK-8205330 This fix will resolve the race in LdapClient where we are explicitly making "null" to LdapClient.conn. Sorry, I don't know this code all that well, but I think that more explanation will be needed before this code can be reviewed. Hi Chris, let me try to explain issue little bit. The issue is a if ldap connection has already been established and then the LDAP directory server sends an (unsolicited) "Notice of Disconnection", the client's processing of this LDAP message can race with an application thread calling new InitialDirContext() to authenticate user credentials (i.e.bind) and throw NPE. I did some analysis and found out that when server send an (unsolicited) "Notice of Disconnection", LdapClient.forceClose() will be called in LdapClient.processUnsolicited() which is called asynchronously by the reader thread in Connection, this means 'LdapClient.conn' may set to null anytime after it received "Notice of Disconnection". The LdapClient and the Connection seem to be loosely coupled. I think part of this is to allow the LdapClient to be GC'ed and finalized separately to the Connection ( that can be reused ). Not setting `conn` to null could have a negative impact on this loose coupling. I also note that the synchronization is implemented poorly in the LdapClient, `conn` is operated on both from within synchronized blocks and from unsynchronized blocks ( which I think is the reason you see the unexpected null ). I agree, not setting 'conn' to null will definitely have some impact. I check the LdapClient and it looks to me it is intentional(i may be wrong) that 'conn' is operated on both from within synchronize blocks and from unsynchronize block. LdapClient, java doc says that connection(conn) take care of it's own sync. ## access from outside LdapClient must sync; * conn - no sync; Connection takes care of its own sync * unsolicited - sync Vector; multiple operations sync'ed ## Please have a look and do let me know your thought on the above. Thanks, Vyom -Chris.
Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize
Hi Rob, Code change looks good to me. Thanks, Vyom On Tuesday 04 September 2018 03:18 AM, Rob McKenna wrote: Hi folks, I'd like to get a re-review of this change: https://bugs.openjdk.java.net/browse/JDK-8139965 http://cr.openjdk.java.net/~robm/8139965/webrev/ In case the original has gone stale: http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042767.html -Rob
Re: [12] RFR 8209773: Refactor shell test javax/naming/module/basic.sh to java
Hi Chris, The refactored java class (RunBasic.java) looks good to me. Thanks, Vyom On Tuesday 28 August 2018 11:20 AM, Chris Yin wrote: Please have a review for below change to refactor shell test javax/naming/module/basic.sh to plain java, no test logic change, old shell script is removed at same time, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8209773 webrev: http://cr.openjdk.java.net/~xyin/8209773/webrev.00/ Regards, Chris
RFR:8205330 InitialDirContext ctor sometimes throws NPE if the server has sent a disconnection
Hi All, Please review this simple fix below webrev: http://cr.openjdk.java.net/~vtewari/8205330/webrev0.0/index.html bugid: https://bugs.openjdk.java.net/browse/JDK-8205330 This fix will resolve the race in LdapClient where we are explicitly making "null" to LdapClient.conn. Thanks, Vyom
Re: RFR: JDK-8176553 LdapContext follows referrals infinitely ignoring set limit
On 8/20/2018 4:19 PM, Chris Hegarty wrote: On 19 Aug 2018, at 12:51, vyom tewari wrote: Hi, Please review the below code change. Webrev : http://cr.openjdk.java.net/~vtewari/8176553/webrev0.0/index.html bugid: https://bugs.openjdk.java.net/browse/JDK-8176553 Our all internal tests are clean, this patch is contributed by Jan Kalina(Redhat). I think the source change is good. How much trouble is it to write a test? We need "LDAP" server, that is why i did not included the test with this patch. I tested locally with "apacheds" and code is working as expected. Thanks, Vyom -Chris.
Re: [12] RFR 8208542: Add 4 JNDI tests to com/sun/jndi/dns/ListTests/
Hi Chris, Latest webrev(.02) looks good to me. One minor comment i will suggest you to expand "setContext" as you did for other JNDI tests. Thanks, Vyom On Friday 10 August 2018 02:34 PM, Chris Yin wrote: Sorry... another minor revision to handle @Override line and imports place, new webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208542/webrev.02/ Regards, Chris On 8 Aug 2018, at 2:51 PM, Chris Yin wrote: Minor revision to address javadoc, initContext() expansion, vararg etc. webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208542/webrev.01/ Regards, Chris On 31 Jul 2018, at 2:39 PM, Chris Yin wrote: Please review the changes to add 4 JNDI tests to com/sun/jndi/dns/ListTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208542 webrev: http://cr.openjdk.java.net/~xyin/8208542/webrev.00/ Regards, Chris
RFR: JDK-8176553 LdapContext follows referrals infinitely ignoring set limit
Hi, Please review the below code change. Webrev : http://cr.openjdk.java.net/~vtewari/8176553/webrev0.0/index.html bugid : https://bugs.openjdk.java.net/browse/JDK-8176553 Our all internal tests are clean, this patch is contributed by Jan Kalina(Redhat). Thanks, Vyom
Re: [12] RFR 8208483: Add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
Hi Chris, Latest code webrev(.03) looks good to me, expending initContext() makes tests more readable :) . Thanks, Vyom On Wednesday 08 August 2018 09:11 AM, Chris Yin wrote: Just one more minor revision to expand initContext() into test method for easier read, new webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208483/webrev.04/ Regards, Chris On 7 Aug 2018, at 5:41 PM, Chris Yin wrote: Hi, Vyom Thanks a lot for your comment, I didn’t realize it, sure, I moved this constant to LookupFactoryBase as you suggested and also abstract the same verify logic into base class as ‘verifyLookupObjectAndValue’, hope that looks better, update new webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208483/webrev.03/ Regards, Chris On 7 Aug 2018, at 4:35 PM, vyom tewari wrote: Hi Chris, Overall latest code looks good to me, one minor comment did you consider moving "private static final String ATTRIBUTE_VALUE = "1.2.4.1";" in "LookupFactoryBase" ? as both LookupWithAnyAttrProp & LookupWithAttrProp inherit "LookupFactoryBase". .Thanks, Vyom On Monday 06 August 2018 03:02 PM, Chris Yin wrote: Hi, Vyom Many thanks for your review and comments, inline and updated webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208483/webrev.01/ On 6 Aug 2018, at 4:12 PM, vyom tewari wrote: Hi Chris, 1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of "getObjectInstance" had to check for null and it will end in lots of boilerplate code. ‘getObjectInstance’ methods are override from DirObjectFactory and ObjectFactory, per my understanding on javadoc, return null should indicate object cannot be created and allow other factories can be tried, feel free to let me know if I misunderstand something on the api doc, thanks Paste a few javadoc here against ‘getObjectInstance’ method as below * @return The object created; null if an object cannot be created. * @exception Exception If this object factory encountered an exception * while attempting to create an object, and no other object factories are * to be tried. 2-> DirTxtFactory.java same as "DirFactory.java”. Same as above 3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for "1.2.4.1" and put one liner comment if possible. This is will help future maintainer to understand why we are comparing with this value. Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre defined attribute value of ‘A’, thanks One generic comment, in most of the places i can see, you declared functions to throw generic exception "Exception", please change it to specific exception or list of specific exceptions if possible. Fixed, those generic exception are generated automatically through override method, I removed those unused throws Exception from xxxFactory. Thanks & Regards, Chris Thanks, Vyom On Monday 30 July 2018 02:08 PM, Chris Yin wrote: Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208483 <https://bugs.openjdk.java.net/browse/JDK-8208483> webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208483/webrev.00/> Regards, Chris
Re: [12] RFR 8208483: Add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
Hi Chris, Overall latest code looks good to me, one minor comment did you consider moving "private static final String ATTRIBUTE_VALUE = "1.2.4.1";" in "LookupFactoryBase" ? as both LookupWithAnyAttrProp & LookupWithAttrProp inherit "LookupFactoryBase". .Thanks, Vyom On Monday 06 August 2018 03:02 PM, Chris Yin wrote: Hi, Vyom Many thanks for your review and comments, inline and updated webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208483/webrev.01/ On 6 Aug 2018, at 4:12 PM, vyom tewari wrote: Hi Chris, 1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of "getObjectInstance" had to check for null and it will end in lots of boilerplate code. ‘getObjectInstance’ methods are override from DirObjectFactory and ObjectFactory, per my understanding on javadoc, return null should indicate object cannot be created and allow other factories can be tried, feel free to let me know if I misunderstand something on the api doc, thanks Paste a few javadoc here against ‘getObjectInstance’ method as below * @return The object created; null if an object cannot be created. * @exception Exception If this object factory encountered an exception * while attempting to create an object, and no other object factories are * to be tried. 2-> DirTxtFactory.java same as "DirFactory.java”. Same as above 3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for "1.2.4.1" and put one liner comment if possible. This is will help future maintainer to understand why we are comparing with this value. Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre defined attribute value of ‘A’, thanks One generic comment, in most of the places i can see, you declared functions to throw generic exception "Exception", please change it to specific exception or list of specific exceptions if possible. Fixed, those generic exception are generated automatically through override method, I removed those unused throws Exception from xxxFactory. Thanks & Regards, Chris Thanks, Vyom On Monday 30 July 2018 02:08 PM, Chris Yin wrote: Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208483 <https://bugs.openjdk.java.net/browse/JDK-8208483> webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208483/webrev.00/> Regards, Chris
Re: [12] RFR 8208483: Add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
On Monday 06 August 2018 03:02 PM, Chris Yin wrote: Hi, Vyom Many thanks for your review and comments, inline and updated webrev as below, thanks http://cr.openjdk.java.net/~xyin/8208483/webrev.01/ On 6 Aug 2018, at 4:12 PM, vyom tewari wrote: Hi Chris, 1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of "getObjectInstance" had to check for null and it will end in lots of boilerplate code. ‘getObjectInstance’ methods are override from DirObjectFactory and ObjectFactory, per my understanding on javadoc, return null should indicate object cannot be created and allow other factories can be tried, feel free to let me know if I misunderstand something on the api doc, thanks Paste a few javadoc here against ‘getObjectInstance’ method as below * @return The object created; null if an object cannot be created. * @exception Exception If this object factory encountered an exception * while attempting to create an object, and no other object factories are * to be tried. Yes you are right, we have to live with null. Vyom 2-> DirTxtFactory.java same as "DirFactory.java”. Same as above 3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for "1.2.4.1" and put one liner comment if possible. This is will help future maintainer to understand why we are comparing with this value. Sure, created constant for “1.2.4.1” and put comment to explain that it’s pre defined attribute value of ‘A’, thanks One generic comment, in most of the places i can see, you declared functions to throw generic exception "Exception", please change it to specific exception or list of specific exceptions if possible. Fixed, those generic exception are generated automatically through override method, I removed those unused throws Exception from xxxFactory. Thanks & Regards, Chris Thanks, Vyom On Monday 30 July 2018 02:08 PM, Chris Yin wrote: Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208483 <https://bugs.openjdk.java.net/browse/JDK-8208483> webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208483/webrev.00/> Regards, Chris
Re: [12] RFR 8208483: Add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/
Hi Chris, 1-> DirAFactory.java, "getIbjectInstance" returns "null" if it fails to construct object. I will suggest you to throw some "RuntimeException" instead returning null. If you return null then caller of "getObjectInstance" had to check for null and it will end in lots of boilerplate code. 2-> DirTxtFactory.java same as "DirFactory.java". 3-> In LookupWithAnyAttrProp/LookupWithAttrProp java create a constant for "1.2.4.1" and put one liner comment if possible. This is will help future maintainer to understand why we are comparing with this value. One generic comment, in most of the places i can see, you declared functions to throw generic exception "Exception", please change it to specific exception or list of specific exceptions if possible. Thanks, Vyom On Monday 30 July 2018 02:08 PM, Chris Yin wrote: Please review the changes to add 5 JNDI tests to com/sun/jndi/dns/FactoryTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208483 <https://bugs.openjdk.java.net/browse/JDK-8208483> webrev: http://cr.openjdk.java.net/~xyin/8208483/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208483/webrev.00/> Regards, Chris
Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/
Hi Chris, Latest webrev looks good to me. Thanks, Vyom On Friday 03 August 2018 02:46 PM, Chris Yin wrote: Hi, Vyom Thank a lot for your review and comments, inline and update new webrev as below http://cr.openjdk.java.net/~xyin/8208279/webrev.02/ On 3 Aug 2018, at 3:45 PM, vyom tewari wrote: Hi Chris, Overall looks good to me, couple of minor comment. 1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with specific exceptions. Fixed, thanks 2-> In couple of places we are calling "removeFromEnvironment" on context and spec says it will return "null" if the env is not set. I see that we are setting the required env in "initContext" so it will never be "null" in our tests, but i suggest you to put just one liner comment that "val" can not be null. This is just to improve code readability, please feel free to ignore, if you think it is not worth enough. Sure, added one line comment before each “removeFromEnvironment” call as you suggested Thanks, Chris Thanks, Vyom On Monday 30 July 2018 08:41 AM, Chris Yin wrote: Please find the new webrev as below, it addressed some similar issues which mentioned in review comments from another thread RFR 8200151, thanks webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/> Regards, Chris On 26 Jul 2018, at 3:37 PM, Chris Yin wrote: Please review the changes to add another 8 JNDI tests to com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208279 <https://bugs.openjdk.java.net/browse/JDK-8208279> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> Regards, Chris
Re: [12] RFR 8208279: Add 8 JNDI tests to com/sun/jndi/dns/EnvTests/
Hi Chris, Overall looks good to me, couple of minor comment. 1-> ProviderUrlGen.java, Line:100 Exception e can be replaced with specific exceptions. 2-> In couple of places we are calling "removeFromEnvironment" on context and spec says it will return "null" if the env is not set. I see that we are setting the required env in "initContext" so it will never be "null" in our tests, but i suggest you to put just one liner comment that "val" can not be null. This is just to improve code readability, please feel free to ignore, if you think it is not worth enough. Thanks, Vyom On Monday 30 July 2018 08:41 AM, Chris Yin wrote: Please find the new webrev as below, it addressed some similar issues which mentioned in review comments from another thread RFR 8200151, thanks webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.01/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.01/> Regards, Chris On 26 Jul 2018, at 3:37 PM, Chris Yin wrote: Please review the changes to add another 8 JNDI tests to com/sun/jndi/dns/EnvTests/ in OpenJDK, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8208279 <https://bugs.openjdk.java.net/browse/JDK-8208279> webrev: http://cr.openjdk.java.net/~xyin/8208279/webrev.00/ <http://cr.openjdk.java.net/~xyin/8208279/webrev.00/> Regards, Chris
Re: [12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/
Hi Chris, Latest code looks good to me. Thanks, Vyom On Friday 27 July 2018 01:12 PM, Chris Yin wrote: Hi, Vyom Thank a lot for your review and comments, inline and update new webrev as below, thanks http://cr.openjdk.java.net/~xyin/8200151/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.01/> On 26 Jul 2018, at 5:27 PM, vyom tewari <mailto:vyom.tew...@oracle.com>> wrote: Hi Chris, Thanks for writing the tests overall code looks good to me, below are few minor comments. 1`-> Fix tag order, my Netbeans always complains :) . Fixed, thanks 2-> you make AuthRecursiveBase class as public but i can not see it is being used outside, i will suggest you to give the default access if possible. Sure, changed it to default access now 3-> AuthTrue.java, change the message as follows "Got exception as expected : " -> "Got expected exception: “; Fixed 4-> PortUnreachable.java , any specific reason for 1000ms or you just choose random value ? Please don't hard-code this specific value create a constant and use it . If possible put some comment why we choose 1 second, this will help in debugging if this test fails intermittently in future. Sure, I created a constant ’THRESHOLD' for it, 1000ms will be used as a threshold value for this test, normally the request should fail very quick (in a few ms), but consider to different platform and test machine performance, we used an acceptable threshold here, some comments added to it in the code too, thanks 5-> Timeout.java, do you really need to set the env using "DNSTestUtils.initEnv" ? I see, this test is not using the DNSServer and other environments variables which were set by "DNSTestUtils.initEnv()" or i am missing something. Actually, it’s indeed not necessary, just set very few default value such as ‘Context.INITIAL_CONTEXT_FACTORY’ in the test code should be fine, but I may still want to keep the test consistency, then we could have better maintainability for those tests. Sorry, I forgot to refactor this test with testbase (actually, its not necessary too, but guess the consistent style and shared base/methods will make the test easy to read and maintain, refactored this test and PortUnreachable.java now) Regards, Chris Thanks, Vyom On Wednesday 25 July 2018 02:41 PM, Chris Yin wrote: Please review the changes to add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/ in OpenJDK, due to known issue 7164518, PortUnreachable.java will be problem list for 'macosx-all', thanks bug: https://bugs.openjdk.java.net/browse/JDK-8200151 <https://bugs.openjdk.java.net/browse/JDK-8200151> webrev: http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/> <http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8200151/webrev.00/>> Regards, Chris
Re: [12] RFR 8200151: Add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/
Hi Chris, Thanks for writing the tests overall code looks good to me, below are few minor comments. 1`-> Fix tag order, my Netbeans always complains :) . 2-> you make AuthRecursiveBase class as public but i can not see it is being used outside, i will suggest you to give the default access if possible. 3-> AuthTrue.java, change the message as follows "Got exception as expected : " -> "Got expected exception: "; 4-> PortUnreachable.java , any specific reason for 1000ms or you just choose random value ? Please don't hard-code this specific value create a constant and use it . If possible put some comment why we choose 1 second, this will help in debugging if this test fails intermittently in future. 5-> Timeout.java, do you really need to set the env using "DNSTestUtils.initEnv" ? I see, this test is not using the DNSServer and other environments variables which were set by "DNSTestUtils.initEnv()" or i am missing something. Thanks, Vyom On Wednesday 25 July 2018 02:41 PM, Chris Yin wrote: Please review the changes to add 8 JNDI tests to com/sun/jndi/dns/ConfigTests/ in OpenJDK, due to known issue 7164518, PortUnreachable.java will be problem list for 'macosx-all', thanks bug: https://bugs.openjdk.java.net/browse/JDK-8200151 <https://bugs.openjdk.java.net/browse/JDK-8200151> webrev: http://cr.openjdk.java.net/~xyin/8200151/webrev.00/ <http://cr.openjdk.java.net/~xyin/8200151/webrev.00/> Regards, Chris
Re: [11] RFR: 8206445: JImageListTest.java failed in Windows
Hi Srinivas,
If runTests() throws exception then "System.gc()" will not be invoked.
Thanks,
Vyom
On Monday 23 July 2018 07:46 PM, Srinivas Dama wrote:
Hi Alan,
As discussed in private conversations, I am pushing below fix for the issue.
webrev: http://cr.openjdk.java.net/~sdama/8206445/webrev.04/
Bug: https://bugs.openjdk.java.net/browse/JDK-8206445
Ran all mach5 tests successfully.
Regards,
Srinivas
- Original Message -
From: alan.bate...@oracle.com
To: srinivas.d...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Monday, 9 July, 2018 4:23:48 PM GMT +05:30 Chennai, Kolkata, Mumbai, New
Delhi
Subject: Re: [11] RFR: 8206445: JImageListTest.java failed in Windows
On 09/07/2018 11:38, Srinivas Dama wrote:
Hi,
Please review
webrev: http://cr.openjdk.java.net/~sdama/8206445/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8206445
testListEmptyFile looks okay but you can reduce the code if if you use
Path.of("."). testListNotAnImage can use Path.of(".") too. Also it can
use Files.writeString which will avoid the test trying to list the
contents of the image while the file is open.
Will you make sure to test this on all platforms before pushing? These
tests have been on and off the exclude list several times in the last
week and it would be good to check that this one is stable before it
runs again.
-Alan
Re: RFR 8207750 : Native handle leak in java.io.WinNTFileSystem.list()
Hi Ivan, looks good to me, although i am not the official reviewer. Thanks, Vyom On Wednesday 18 July 2018 05:31 AM, Ivan Gerasimov wrote: Hello! In the file WinNTFileSystem_md.c there is a potential leak of native handle obtained from FindFirstFileW() in the unfortunate event of memory allocation failure. Would you please help review a simple fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8207750 WEBREV: http://cr.openjdk.java.net/~igerasim/8207750/00/webrev/ Thanks in advance!
Re: RFR 8198882: Add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/
Hi Chris, latest webrev looks good to me, thanks for explanation about copyright date. Thanks, Vyom On Friday 13 July 2018 11:44 AM, Chris Yin wrote: Hi, Vyom Thank you for the review and comments, update webrev as below and comment inline webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.02/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.02/> On 13 Jul 2018, at 1:46 PM, vyom tewari <mailto:vyom.tew...@oracle.com>> wrote: Hi Chris, Thanks for doing this overall looks good to me, few minor comments. 1-> DNSTestUtils.java, please start the server first and then set the "TEST_DNS_SERVER_THREAD". This will not make much difference but we will avoid setting "TEST_DNS_SERVER_THREAD" env variable if server fails to start. 129 env.put(TEST_DNS_SERVER_THREAD, inst); 130 inst.start(); Fixed, thanks 2-> I noticed that copyright date (Copyright (c) 2000, 2018,) but webrev tells all the tests are new, please fix copyright date as well. Thanks for checking this. Since this task is part of umbrella enhancement JDK-8191011 <https://bugs.openjdk.java.net/browse/JDK-8191011> JNDI SQE tests co-location, for those added tests which are migrated from SQE tests, the copyright date will follow the guidance SQE Test copyright year + migration copyright year if the 2 year are not same, for dump files (like *.dns) are new added under our new framework so just use current copyright year, hope that explains :), thanks Regards, Chris Thanks, Vyom On Thursday 12 July 2018 02:08 PM, Chris Yin wrote: Please have a review to new webrev as below, some code refactoring on lib parts and enhanced DNSServer to handle retry request which will make the tests more stable, thanks http://cr.openjdk.java.net/~xyin/8198882/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.01/> Regards, Chris On 22 Mar 2018, at 11:16 AM, Chris Yin <mailto:xu.y@oracle.com>> wrote: Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8198882 webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/> Regards, Chris
Re: RFR 8198882: Add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/
Hi Chris, Thanks for doing this overall looks good to me, few minor comments. 1-> DNSTestUtils.java, please start the server first and then set the "TEST_DNS_SERVER_THREAD". This will not make much difference but we will avoid setting "TEST_DNS_SERVER_THREAD" env variable if server fails to start. 129 env.put(TEST_DNS_SERVER_THREAD, inst); 130 inst.start(); 2-> I noticed that copyright date (Copyright (c) 2000, 2018,) but webrev tells all the tests are new, please fix copyright date as well. Thanks, Vyom On Thursday 12 July 2018 02:08 PM, Chris Yin wrote: Please have a review to new webrev as below, some code refactoring on lib parts and enhanced DNSServer to handle retry request which will make the tests more stable, thanks http://cr.openjdk.java.net/~xyin/8198882/webrev.01/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.01/> Regards, Chris On 22 Mar 2018, at 11:16 AM, Chris Yin <mailto:xu.y@oracle.com>> wrote: Please review the changes to add 10 JNDI tests to com/sun/jndi/dns/AttributeTests/, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8198882 webrev: http://cr.openjdk.java.net/~xyin/8198882/webrev.00/ <http://cr.openjdk.java.net/%7Exyin/8198882/webrev.00/> Regards, Chris
Re: RFR 8207060 : Memory leak when malloc fails within WITH_UNICODE_STRING block
Hi Ivan, Changes looks good to me, nice cleanup. Thanks, Vyom On Wednesday 11 July 2018 09:45 PM, Ivan Gerasimov wrote: Hello! File src/java.base/windows/native/libjava/io_util_md.c In the function pathToNTPath(), memory is allocated with malloc() within a block of macros WITH_UNICODE_STRING / END_UNICODE_STRING. In an unlikely event of malloc() failure, the function returns, failing to release the string array acquired with WITH_UNICODE_STRING. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8207060 WEBREV: http://cr.openjdk.java.net/~igerasim/8207060/00/webrev/ Thanks in advance!
Re: [JDK 11] RFR 8187069: The case auto failed with the "java.lang.ClassNotFoundException: IPv6NameserverPlatformParsingTest" exception
looks good to me. Vyom On Friday 29 June 2018 06:22 AM, Chris Yin wrote: Hi, Vyom Sure, fixed the tag order as you suggested, thanks New changes: diff -r 1308189b0848 test/jdk/com/sun/jndi/dns/Test6991580.java --- a/test/jdk/com/sun/jndi/dns/Test6991580.javaThu Jun 28 17:45:59 2018 -0700 +++ b/test/jdk/com/sun/jndi/dns/Test6991580.javaFri Jun 29 08:48:05 2018 +0800 @@ -33,10 +33,11 @@ /* * @test * @bug 6991580 8080108 8133035 - * @requires os.family != "windows" * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException * @modules java.desktop * jdk.naming.dns/com.sun.jndi.dns + * @requires os.family != "windows" + * @build IPv6NameserverPlatformParsingTest * @run main/manual Test6991580 */ Regards, Chris On 28 Jun 2018, at 7:02 PM, vyom tewari <mailto:vyom.tew...@oracle.com>> wrote: Hi Chris, change looks good to me. My NetBeans always complains about tag order if it is not correct, as you adding the new tag i will suggest you to please fix the tag order as well. /* * @test * @bug 6991580 8080108 8133035 * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException * @modules java.desktop * jdk.naming.dns/com.sun.jndi.dns * @requires os.family != "windows" * @build IPv6NameserverPlatformParsingTest * @run main/manual Test6991580 */ Thanks, Vyom On Thursday 28 June 2018 07:31 AM, Chris Yin wrote: Please review below one line change for manual test case com/sun/jndi/dns/Test6991580.java to build test class automatically which will be used in manual steps, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8187069 Review change as below: diff -r 2d3e99a72541 test/jdk/com/sun/jndi/dns/Test6991580.java --- a/test/jdk/com/sun/jndi/dns/Test6991580.javaWed Jun 27 17:02:41 2018 -0700 +++ b/test/jdk/com/sun/jndi/dns/Test6991580.javaThu Jun 28 09:50:37 2018 +0800 @@ -37,6 +37,7 @@ * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException * @modules java.desktop * jdk.naming.dns/com.sun.jndi.dns + * @build IPv6NameserverPlatformParsingTest * @run main/manual Test6991580 */ Regards, Chris
Re: [JDK 11] RFR 8187069: The case auto failed with the "java.lang.ClassNotFoundException: IPv6NameserverPlatformParsingTest" exception
Hi Chris, change looks good to me. My NetBeans always complains about tag order if it is not correct, as you adding the new tag i will suggest you to please fix the tag order as well. /* * @test * @bug 6991580 8080108 8133035 * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException * @modules java.desktop * jdk.naming.dns/com.sun.jndi.dns * @requires os.family != "windows" * @build IPv6NameserverPlatformParsingTest * @run main/manual Test6991580 */ Thanks, Vyom On Thursday 28 June 2018 07:31 AM, Chris Yin wrote: Please review below one line change for manual test case com/sun/jndi/dns/Test6991580.java to build test class automatically which will be used in manual steps, thanks bug: https://bugs.openjdk.java.net/browse/JDK-8187069 Review change as below: diff -r 2d3e99a72541 test/jdk/com/sun/jndi/dns/Test6991580.java --- a/test/jdk/com/sun/jndi/dns/Test6991580.javaWed Jun 27 17:02:41 2018 -0700 +++ b/test/jdk/com/sun/jndi/dns/Test6991580.javaThu Jun 28 09:50:37 2018 +0800 @@ -37,6 +37,7 @@ * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException * @modules java.desktop * jdk.naming.dns/com.sun.jndi.dns + * @build IPv6NameserverPlatformParsingTest * @run main/manual Test6991580 */ Regards, Chris
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
On Friday 25 May 2018 11:19 AM, Ivan Gerasimov wrote:
Hi Wiijun!
On 5/24/18 10:13 PM, Weijun Wang wrote:
On May 25, 2018, at 11:58 AM, Ivan Gerasimov
<ivan.gerasi...@oracle.com> wrote:
I also wonder whether a smart compiler might not flag code where
the errors do infact have the same value:
if (errno == 11 || errno == 11) ...
At least gcc -O completely removes the second redundant test, so no
observable changes is expected on supported platforms.
And it silently compiles without showing any warning, right? Good if
yes.
--Max
Yep, all is good.
I've built/tested the patched JDK on all supported platforms with no
issues.
And we already have places, where both EAGAIN and EWOULDBLOCK are used
in one if clause (as I just replied to David):
java.base/unix/native/libnet/SocketInputStream.c, in
NET_ReadWithTimeout():
result = NET_NonBlockingRead(fd, bufP, len);
if (result == -1 && ((errno == EAGAIN) || (errno ==
EWOULDBLOCK))) {
i wrote this code and that time even i wanted to fix the other native
code (check error code for both EAGAIN & EWOULDBLOCK) as you did ,but i
did not .
So the fix basically proposes to use this approach consistently.
we can at least fix the native part of code.
Vyom
With kind regards,
Ivan
Re: [11] RFR: JDK-8202582 : DateTimeFormatterBuilder.parseOffsetBased unnecessarily calls toString()
looks good to me. Vyom On Friday 04 May 2018 02:29 PM, Rachna Goel wrote: Hi, Kindly review this small patch for JDK-8202582. https://bugs.openjdk.java.net/browse/JDK-8202582 http://cr.openjdk.java.net/~rgoel/JDK-8202582/webrev/ Fix is to call text.subSequence() before calling toString() on input string, for more performance.
Re: RFR: [PATCH] 8176553 Fix LDAP referral loop
On 4/4/2018 8:59 PM, Jan Kalina wrote:
Note: Test is not included, as it would require running LDAP server.
(existing ldap tests in JDK use only mocks of the server)
The patch was manually verified using reproducer attached to issue.
Hi Jan,
I ran the reproducer long back on my Linux box(Ubuntu 1604) on apacheDS
2 and it was not reproducible at my end. can you please let us know your
environment detail.
Thanks,
Vyom
On Wed, Apr 4, 2018 at 4:12 PM, Jan Kalina <jkal...@redhat.com> wrote:
Hi,
I has prepared trivial patch for bug JDK-8176553,
which I would like to get reviewed and sponsored.
I am covered by Red Hat OCA.
The bug affects upstream, JDK9 and JDK8 too.
The reproducer is available in issue tracker:
https://bugs.openjdk.java.net/browse/JDK-8176553
PATCH:
--
diff --git
a/src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
b/src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
---
a/src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
+++
b/src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
@@ -312,7 +312,8 @@
if ((refEx != null) &&
(refEx.hasMoreReferrals() ||
- refEx.hasMoreReferralExceptions())) {
+ refEx.hasMoreReferralExceptions()) &&
+ ! (errEx instanceof LimitExceededException)) {
if (homeCtx.handleReferrals == LdapClient.LDAP_REF_THROW) {
throw (NamingException)(refEx.fillInStackTrace());
--
Thanks,
Jan Kalina
Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider
Hi Rob,
Latest code looks good to me.
minor bit.
LdapDnsProviderService.java
Line: 71 , while loop condition is bit complex, we can simplify it little bit
as follows.This will make code more readable and easy to understand.
while (iterator.hasNext())
{
LdapDnsProvider p = iterator.next();
result = p.lookupEndpoints(url, env);
if(result != null && !result.getEndpoints().isEmpty()){
break;
}
}
It is just a personal preference you can ignore it if you don't like it.
Thanks,
Vyom
On Wednesday 06 December 2017 11:54 PM, Rob McKenna wrote:
Thanks Vyom, these should be fixed in:
http://cr.openjdk.java.net/~robm/8160768/webrev.07/
Comments inline:
On 06/12/17 22:14, vyom tewari wrote:
Hi Rob,
Please find below comments.
DefaultLdapDnsProvider.java
line 35: convert it to for-each code will be more readable.
LdapDnsProviderService.java
line 21: constant name does not follow Java naming convention, there are
other places as well please fix it.
getInstance() is already synchronized why you need another "synchronized"
block ?
Ah, neglected to remove the outer synchronized keyword, thanks.
Line 70: Please use result.getEndpoints().isEmpty() in place of
result.getEndpoints().size() == 0
LdapDnsProvider.java
constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does
nothing, can you explain why LdapDnsProvider() calls LdapDnsProvider(Void
unused) ?
I've copied this pattern from the System.LoggerFinder api and I had
forgotten what it was for too:
http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
Section 7.3.
LdapDnsProviderResult.java
Private field domainName & endpoints both can be final.
LdapDnsProviderTest.java
Fix the tag Order it is not correct. correct order is as follows.
/**
* @test
* @bug 8160768
* @summary ctx provider tests for ldap
* @modules java.naming/com.sun.jndi.ldap
* @compile dnsprovider/TestDnsProvider.java
* @run main/othervm LdapDnsProviderTest
* @run main/othervm LdapDnsProviderTest nosm
* @run main/othervm LdapDnsProviderTest smnodns
* @run main/othervm LdapDnsProviderTest smdns
*/
Line 82,83 you don't need System.out.println(e); e.printStackTrace();
I'm going to leave these in as I've had a few failing tests in the past
where I've needed to push diagnostic changes like this to determine the
root cause.
-Rob
Line 70: you don't need this try block
Line 96 : constant name does not follow Java naming convention
Line 105: you can use try-with-resources this will avoid some boilerplate.
Thanks,
Vyom
On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
As this enhancement is limited to JDK10 this frees us up to explore a
different approach.
http://cr.openjdk.java.net/~robm/8160768/webrev.06/
In this webrev we're using the Service Provider Interface to load an
implementation of the new LdapDnsProvider class. Implementations of this
class are responsible for resolving DNS endpoints for a given ldap url
should the default implementation be insufficient in a particular
environment.
Note: if a security manager is installed the ldapDnsProvider
RuntimePermission must be granted.
-Rob
Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider
Hi Rob, Please find below comments. DefaultLdapDnsProvider.java line 35: convert it to for-each code will be more readable. LdapDnsProviderService.java line 21: constant name does not follow Java naming convention, there are other places as well please fix it. getInstance() is already synchronized why you need another "synchronized" block ? Line 70: Please use result.getEndpoints().isEmpty() in place of result.getEndpoints().size() == 0 LdapDnsProvider.java constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which does nothing, can you explain why LdapDnsProvider() calls LdapDnsProvider(Void unused) ? LdapDnsProviderResult.java Private field domainName & endpoints both can be final. LdapDnsProviderTest.java Fix the tag Order it is not correct. correct order is as follows. /** * @test * @bug 8160768 * @summary ctx provider tests for ldap * @modules java.naming/com.sun.jndi.ldap * @compile dnsprovider/TestDnsProvider.java * @run main/othervm LdapDnsProviderTest * @run main/othervm LdapDnsProviderTest nosm * @run main/othervm LdapDnsProviderTest smnodns * @run main/othervm LdapDnsProviderTest smdns */ Line 82,83 you don't need System.out.println(e); e.printStackTrace(); Line 70: you don't need this try block Line 96 : constant name does not follow Java naming convention Line 105: you can use try-with-resources this will avoid some boilerplate. Thanks, Vyom On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote: As this enhancement is limited to JDK10 this frees us up to explore a different approach. http://cr.openjdk.java.net/~robm/8160768/webrev.06/ In this webrev we're using the Service Provider Interface to load an implementation of the new LdapDnsProvider class. Implementations of this class are responsible for resolving DNS endpoints for a given ldap url should the default implementation be insufficient in a particular environment. Note: if a security manager is installed the ldapDnsProvider RuntimePermission must be granted. -Rob
Re: Fix inconsistent behavior of java.nio.file.attribute.BasicFileAttributes.lastModifiedTime()
Hi Martin,
which specific Operating system are you observing this issue ?. Mostly
all modern OS's supports higher precision time stamp.
Thanks,
Vyom
On Friday 24 November 2017 08:23 PM, Schaef, Martin wrote:
We are experiencing a problem with the following test case:
https://github.com/eclipse/jgit/blob/master/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java#L364
Depending on which gcc version we use to compile OpenJDK, the assertion in
https://github.com/eclipse/jgit/blob/master/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CheckoutCommandTest.java#L397
fails.
That is, the methods
java.nio.file.attribute.BasicFileAttributes.lastModifiedTime()
and java.io.File.lastModified() return results with a different precision.
We root-caused it to the following code block in UnixNativeDispatcher.c:
http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/07011844584f/src/solaris/native/sun/nio/fs/UnixNativeDispatcher.c#l456
The test case at the bottom of this email can be used to reproduce the
behavior. When using a JDK compiled with gcc 4.1.2, the test prints:
nio 151138718
io 151138718
But when using a JDK compiled with gcc 4.4.6, the code returns:
nio 1511387187817
io 1511387187000
Exception in thread "main" java.lang.RuntimeException: 1511387187817 !=
1511387187000
at Issue225.testLastModified(Issue225.java:33)
at Issue225.main(Issue225.java:11)
In comparison, the OracleJDK binaries as downloaded from the website behave
like the gcc 4.1.2 compiled binaries. To avoid confusion, and to ensure that
both methods behave the same, we propose to remove the block in
UnixNativeDispatcher.c (see attached patch).
Cheers,
Martin
### Test code to witness the behavior difference ###
import java.io.File;
import java.io.IOException;
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributeView;
import java.nio.file.attribute.BasicFileAttributes;
public class Issue225 {
public static void main(String[] args) throws IOException {
(new Issue225()).testLastModified();
}
public void testLastModified() throws IOException {
File file = File.createTempFile("Test", ".txt");
file.deleteOnExit();
Path nioPath = file.toPath();
BasicFileAttributes readAttributes = nioPath
.getFileSystem().provider()
.getFileAttributeView(nioPath,
BasicFileAttributeView.class,
LinkOption.NOFOLLOW_LINKS
).readAttributes();
System.out.println(readAttributes.getClass());
System.out.println("nio " + readAttributes.lastModifiedTime().toMillis());
System.out.println("io " + file.lastModified() );
if (readAttributes.lastModifiedTime().toMillis() != file.lastModified()) {
throw new RuntimeException(readAttributes.lastModifiedTime().toMillis()
+ " != "
+ file.lastModified());
}
}
}
Re: RFR 8145635 : Add TCP_QUICKACK socket option
Hi Roger,
Thanks for the review, i incorporated all review comments from you
except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for the
option to omit without
embedding the name."). ExtendedSocketOptions.java is part of
"jdk.net" so we can not directly use it in java.base, hence i used the
option name to filter "TCP_QUICKACK".
Please find the update
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).
Thanks,
Vyom
On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:
Hi Vyom,
Comments:
- update copyright
- use @since 18.3 instead of @since 10
- ExtendedSocketOptions.java: 265,269 include the "TCP_QUICKACK" in
the exception messages.
Line 144: if you are going to keep the assert, add a explanation
about how it could happen.
I'd remove the assert.
- The first sentence should be a complete sentence: "TCP_QUICKACK
socket option enables sending TCP/IP acks immediately" or similar.
- A reference to the appropriate TCP/IP spec for quickack would be a
good addition. At least the RFC # and section.
- 85: space after "." The phrasing is a bit odd in places in the
javadoc.
- line 81/82 the value is true to enable and false to disable.
- This phrase is confusing: "it only enables a switch to or from
TCP_QUICKACK mode";
Since it is set on a socket, it should remain set on that socket
until it is changed.
- 203: be consistent in using enable/disable in parameters, etc even
for private methods.
"on" -> "enable"
PlainDatagramSocketImpl: 89:
Why create a new set with zero or one items just to throw it away?
Use the iterator to add only the non-TCP_QUICKACK options to the
supported options.
Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for the
option to omit without
embedding the name.
Sockets.java:
- The initialization of isQuickAckAvailable might be cleaner as an
nested static class
that initializes the value in its static initializer. That would
delay the init til needed
and avoid extra flags.
LinuxSocketOptions.java:
- the native methods should be static; since the instance is unused.
- line 41: the return type should be Boolean
- line 46: the return type of getQuickAck0 should be Boolean like the
argument to set.
- line 34: using JNU_ThrowByNameWithLastError directly is
sufficient; if the errno does not have a string unix supplies "unknown
error nnn".
Regards, Roger
On 10/10/2017 2:58 PM, Chris Hegarty wrote:
Vyom,
What about suggestion 1) below, the name of the socket option?
-Chris.
On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi Chris,
Thanks for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html).
I incorporated review comments from you and re-base the patch to our
consolidated repo(jdk10/master).
Thanks,
Vyom
On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:
Vyom,
On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:
Hi All,
As jdk.net API already moved out of java.base, Please review the
below code change for jdk10.
Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev:
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html
This looks quite good. Some comments:
1) I wonder if we should just call the option TCP_QUICKACK, rather
than SO_QUICKACK? Similar to TCP_NODELAY.
2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1)
above.
3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint,
rather than jobject, thus avoiding the need for createBoolean. The
conversation can happen in the Java layer. Can you please reduce
the lint length in this file, by wrapping similar to the style of
the Solaris version.
4) ExtendedSocketOptions.java
- drop the , they are unnecessary.
- I think, similar to TCP_NODELAY, the spec should say that the
options is TCP specific. From TCP_NODELAY: "The socket option is
specific to stream-oriented sockets using the TCP/IP protocol.”
- "In TCP_QUICKACK mode”, maybe “When the option is enabled…”
-Chris.
