Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v7]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge - Merge master - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=06 Stats: 86 lines in 10 files changed: 12 ins; 29 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v6]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge master - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=05 Stats: 86 lines in 10 files changed: 12 ins; 29 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
On Wed, 1 Jun 2022 21:09:15 GMT, Mandy Chung wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> back to wait 1 second > > No, it doesn't work. You can build a fastdebug build with `configure > --enable-debug`. I reproduce it on macOS. If I restore to the previous > version without 8287384, the test passes. Inspired by @mlchung (See https://github.com/openjdk/jdk/pull/9021), use less waiting time for UnloadingTest and re-open this PR. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v5]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=04 Stats: 87 lines in 10 files changed: 19 ins; 25 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: JDK-8287671: Adjust ForceGC to invoke System::gc fewer times for negative case
On Fri, 3 Jun 2022 18:05:52 GMT, Mandy Chung wrote: > This reapplies JDK-8287384 and adjust the number of GCs for negative case, > i.e. the condition is never met that is used to verify a reference is not > GC'ed. Marked as reviewed by xuelei (Reviewer). @mlchung Thank you for pick up and fix the issue. The waiting time now is reduced from 10 seconds to 1 seconds. As most of the tests intermittent fail, the failure may be more popular now. Alternatively, it might be OK to run less cases in UnloadingTest.java (for example one or two call to tryUnload() in one test by separating the case into a few). But let's see if the intermittent failure is still rare enough. - PR: https://git.openjdk.java.net/jdk/pull/9021
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v4]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: rollback is not in this branch - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/a8768e09..4a80db95 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=02-03 Stats: 58 lines in 1 file changed: 17 ins; 24 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Withdrawn: 8287596: Reorg jdk.test.lib.util.ForceGC
On Wed, 1 Jun 2022 19:08:03 GMT, Xue-Lei Andrew Fan wrote: > This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v3]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: rollback JDK-8287384 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/f3d9eb82..a8768e09 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=01-02 Stats: 55 lines in 1 file changed: 21 ins; 14 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
On Wed, 1 Jun 2022 21:07:16 GMT, Xue-Lei Andrew Fan wrote: >> This is a follow up update per comments in [JDK-8287384 >> PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in >> open part looks good to me. Please help to run Mach5 just case the closed >> test cases are impacted. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > back to wait 1 second My macOS (M1) and Linux (CentOS) may be too powerful to reproduce the failure. I tried to set the JTREG time out to 60 seconds, and the timeout issue could be reproduced. In `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java`, there are 6 calls to the method `assertFalse(unloader.tryUnload())`. Each call to tryUnload() takes at least 10 seconds. So the 6 calls takes 60 seconds at least. If I set the regression timeout value to 70 seconds, the test still can pass. It implies the rest part other than tryUnload() is pretty fast. It looks like a regression introduced with the update for [JDK-8287384](https://bugs.openjdk.java.net/browse/JDK-8287384). I did not fine the cause yet. I will have more checking tomorrow. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC
On Wed, 1 Jun 2022 20:45:07 GMT, Mandy Chung wrote: > JDK-8287384 causes > `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java` to timeout > when running with fastdebug VM. I think this might be caused by more frequent > GCs. > > I tried your patch and the test fails. I updated the waiting time back to 1 second. Would you please check again? > JDK-8287384 causes > `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java` to timeout > when running with fastdebug VM. I think this might be caused by more frequent > GCs. > > I tried your patch and the test fails. I updated the waiting time back to 1 second. Would you please check again? - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: back to wait 1 second - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/a1d91596..f3d9eb82 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
RFR: 8287596: Reorg jdk.test.lib.util.ForceGC
This is a follow up update per comments in [JDK-8287384 PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in open part looks good to me. Please help to run Mach5 just case the closed test cases are impacted. - Commit messages: - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8979&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287596 Stats: 83 lines in 10 files changed: 12 ins; 40 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei Sorry, I was working on a history webpage while submit the PR and did not notice new comments. I will address the missed comments in the following up RFE. - PR: https://git.openjdk.java.net/jdk/pull/8907
Integrated: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: d5b6c7bd Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/d5b6c7bde1ae1ddcc9ad31b99480b67a913ff20a Stats: 21 lines in 1 file changed: 8 ins; 1 del; 12 mod 8287384: Speed up jdk.test.lib.util.ForceGC Reviewed-by: rriggs, bchristi, dfuchs, mchung - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 21:40:42 GMT, Xue-Lei Andrew Fan wrote: > > Even using a Cleaner is a more overhead than necessary. I would have > > skipped the overhead of a cleaner and Atomic classes with something more > > self contained as a static method: > > I agreed that the using of Cleaner is still heavy, but it may be acceptable > as the code is for testing only. If a new static method is introduced, test > cases using the current API would also need update so as to benefit from it. > I was hesitate for test cases update as it may involve more evaluations (and > risks). But let me check. I evaluated the use of ForceGC (10+ use). It looks like that the update should be straightforward for most of them. Most testing should be placed in case I missed something. I would like to file a new RFE and submit this PR, so that if something is wrong with new update, we could rollback to this. - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 21:22:23 GMT, Roger Riggs wrote: > Even using a Cleaner is a more overhead than necessary. I would have skipped > the overhead of a cleaner and Atomic classes with something more self > contained as a static method: I agreed that the using of Cleaner is still heavy, but it may be acceptable as the code is for testing only. If a new static method is introduced, test cases using the current API would also need update so as to benefit from it. I was hesitate for test cases update as it may involve more evaluations (and risks). But let me check. - PR: https://git.openjdk.java.net/jdk/pull/8907
RFR: 8287384: Speed up ForceGC
Hi, May I have this test update reviewed? The ForceGC could be enhanced by using smaller wait/sleep time, and shared cleaner. Thanks, Xuelei - Commit messages: - 8287384: Speed up ForceGC Changes: https://git.openjdk.java.net/jdk/pull/8907/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8907&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287384 Stats: 21 lines in 1 file changed: 8 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8907/head:pull/8907 PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Thu, 19 May 2022 09:31:07 GMT, Kevin Walls wrote: >> Replaces usages of articles that follow each other in all combinations: >> a/the, an?/an?, the/the… >> >> Also, I fixed a couple of spelling mistakes. > > test/jdk/sun/security/tools/jarsigner/OldSig.java line 32: > >> 30: /* >> 31: * See also PreserveRawManifestEntryAndDigest.java for tests with >> arbitrarily >> 32: * formatted individual sections in addition the main attributes tested > > "in addition to the..." +1. - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. The security/crypto parts look good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). The update in security area looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8283800: Simplify String.indexOf/lastIndexOf calls
On Sun, 20 Mar 2022 12:45:34 GMT, Andrey Turbanov wrote: > In a few places String.indexOf/lastIndexOf methods are called with default > parameter for index: `0` for `indexOf`, length() for `lastIndexOf`. > I propose to cleanup such calls. It makes code a bit easier to read. In case > of `indexOf` it even could be faster, as there is separate intrinsic for > `indexOf` call without index argument. Please add a noreg- label to the bug. Otherwise, looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7877
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Wed, 23 Mar 2022 21:59:02 GMT, Valerie Peng wrote: >> I see. >> >> Would you mind add a comment about the provider loading impact, just in case >> someone else have similar questions in the future? > > Sure, I can do that. Will add a comment about this. Thank you. I have no more comment except a minor nit. - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v8]
On Wed, 23 Mar 2022 22:48:41 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Added comment regarding possible deadlocks. Looks good to me. Thanks! src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java line 137: > 135: public static final int DEF_ED_KEY_SIZE; > 136: public static final int DEF_XEC_KEY_SIZE; > 137: // the logic for finding the max allowable value in > getDefAESKeySize() Capital the 1st letter? - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Wed, 23 Mar 2022 20:45:22 GMT, Valerie Peng wrote: >> src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java >> line 129: >> >>> 127: return currVal; >>> 128: } >>> 129: >> >> I'm not very sure of this method. Is it performance friendly if making the >> default key size calculation in the static block (from line 142 to the end >> of the file)? Then, the DEF_AES_KEY_SIZE could be a public primitive int. >> >> Or did I miss something? > > My very first prototype is to implement the AES keysize calculation as you > commented, i.e. in the static block and use an int for DEF_AES_KEY_SIZE. > However, it is later discovered through testing that this leads to deadlocks > as this interferes with provider loading. Given that AES key size is just a > small piece of the whole puzzle, it seems safer to defer this to a later > point when it's actually needed rather than touching the whole provider > loading logic just to make this a static int. Performance-wise, this is a > very small piece, generally should just be the AtomicInteger.get(). I see. Would you mind add a comment about the provider loading impact, just in case someone else have similar questions in the future? - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v7]
On Tue, 22 Mar 2022 21:25:28 GMT, Valerie Peng wrote: >> It's been several years since we increased the default key sizes. Before >> shifting to PQC, NSA replaced its Suite B cryptography recommendations with >> the Commercial National Security Algorithm Suite which suggests: >> >> - SHA-384 for secure hashing >> - AES-256 for symmetric encryption >> - RSA with 3072 bit keys for digital signatures and for key exchange >> - Diffie Hellman (DH) with 3072 bit keys for key exchange >> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures >> (ECDSA) >> >> So, this proposed changes made the suggested key size and algorithm changes. >> The changes are mostly in keytool, jarsigner and their regression tests, so >> @wangweij Could you please take a look? >> >> Thanks! > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Minor code refactoring src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java line 129: > 127: return currVal; > 128: } > 129: I'm not very sure of this method. Is it performance friendly if making the default key size calculation in the static block (from line 142 to the end of the file)? Then, the DEF_AES_KEY_SIZE could be a public primitive int. Or did I miss something? src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java line 137: > 135: public static final int DEF_ED_KEY_SIZE; > 136: public static final int DEF_XEC_KEY_SIZE; > 137: private static final AtomicInteger DEF_AES_KEY_SIZE; See the previous comment, maybe it could be `public static final int DEF_AES_KEY_SIZE.` - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8283426: Fix 'exeption' typo
On Sun, 20 Mar 2022 13:30:01 GMT, Andrey Turbanov wrote: > Fix repeated type `exeption` Looks good to me. Thanks! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7879
Re: RFR: 8258796: [test] Apply HexFormat to tests for java.security [v2]
On Fri, 8 Jan 2021 16:26:09 GMT, Roger Riggs wrote: >> Followup to the addition of java.util.HexFormat. >> Uses of adhoc hexadecimal conversions are replaced with HexFormat. >> Redundant utility methods are modified or removed. >> In a few places the data being printed is ASN.1 and the test utility >> HexPrinter with the ASNFormatter is added. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyrights to 2021 Looks good to me. Thank you! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2006
Re: RFR: 8039278: console.sh failed Automatically with exit code 1
On Thu, 7 Jan 2021 18:26:12 GMT, Rajan Halade wrote: > 8039278: console.sh failed Automatically with exit code 1 Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1981
Re: RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources [v3]
On Wed, 2 Dec 2020 18:42:36 GMT, Christoph Langer wrote: >> test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 37: >> >>> 35: * will not leave leaking socket file descriptors >>> 36: * @library /test/lib >>> 37: * @run main/othervm SSLSocketLeak >> >> See bellow comment, I may suggest to have it as a manual test case if you >> agree the test case could be impacted. >> @run main/manual SSLSocketLeak > > Hm, I think it's fine as it is. Running it in othervm will make sure the test > runs in its own vm (see http://openjdk.java.net/jtreg/command-help.html). So > within the VM process there should not be any interference by other workload. > And we check open files before testing and afterwards, and allow for some > margin. > > The test has been running in our test setup for several days now, so I think > it should be ok. And if worst comes to worse, and we see test noise, we might > change the test to manual later on. Sounds good to me. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1363
Re: RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources [v3]
On Sun, 22 Nov 2020 18:27:56 GMT, Christoph Langer wrote: >> There is a flaw in sun.security.ssl.SSLSocketImpl::close() which leads to >> leaking socket resources after JDK-8224829. >> >> The close method calls duplexCloseOutput() and duplexCloseInput(). In case >> of an exception in any of these methods, the call to closeSocket() is >> bypassed, and the underlying Socket may not be closed. >> >> This manifests in a real life leak after JDK-8224829 has introduced a call >> to getSoLinger() on the path of duplexCloseOutput -> closeNotify. If socket >> impl / OS socket hadn't been created yet it is done at that place. But then >> after duplexCloseOutput eventually fails with a SocketException since the >> socket wasn't connected, closing fails to call Socket::close(). >> >> This problem can be reproduced by this code: >> SSLSocket sslSocket = >> (SSLSocket)SSLSocketFactory.getDefault().createSocket(); >> sslSocket.getSSLParameters(); >> sslSocket.close(); >> >> This is what happens when SSLContext.getDefault().getDefaultSSLParameters() >> is called, with close() being eventually called by the finalizer. >> >> I'll open this PR as draft for now to start discussion. I'll create a >> testcase to reproduce the issue and add it soon. >> >> I propose to modify the close method such that duplexClose is only done on a >> connected/bound socket. Maybe it even suffices to only do it when connected. >> >> Secondly, I'm proposing to improve exception handling a bit. So in case >> there's an IOException on the path of duplexClose, it is caught and logged. >> But the real close moves to the finally block since it should be done >> unconditionally. > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Small test improvement Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1363
Re: RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources [v3]
On Sun, 22 Nov 2020 18:27:56 GMT, Christoph Langer wrote:
>> There is a flaw in sun.security.ssl.SSLSocketImpl::close() which leads to
>> leaking socket resources after JDK-8224829.
>>
>> The close method calls duplexCloseOutput() and duplexCloseInput(). In case
>> of an exception in any of these methods, the call to closeSocket() is
>> bypassed, and the underlying Socket may not be closed.
>>
>> This manifests in a real life leak after JDK-8224829 has introduced a call
>> to getSoLinger() on the path of duplexCloseOutput -> closeNotify. If socket
>> impl / OS socket hadn't been created yet it is done at that place. But then
>> after duplexCloseOutput eventually fails with a SocketException since the
>> socket wasn't connected, closing fails to call Socket::close().
>>
>> This problem can be reproduced by this code:
>> SSLSocket sslSocket =
>> (SSLSocket)SSLSocketFactory.getDefault().createSocket();
>> sslSocket.getSSLParameters();
>> sslSocket.close();
>>
>> This is what happens when SSLContext.getDefault().getDefaultSSLParameters()
>> is called, with close() being eventually called by the finalizer.
>>
>> I'll open this PR as draft for now to start discussion. I'll create a
>> testcase to reproduce the issue and add it soon.
>>
>> I propose to modify the close method such that duplexClose is only done on a
>> connected/bound socket. Maybe it even suffices to only do it when connected.
>>
>> Secondly, I'm proposing to improve exception handling a bit. So in case
>> there's an IOException on the path of duplexClose, it is caught and logged.
>> But the real close moves to the finally block since it should be done
>> unconditionally.
>
> Christoph Langer has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Small test improvement
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 57:
> 55: if ((fds_end - fds_start) > (NUM_TEST_SOCK / 10)) {
> 56: throw new RuntimeException("Too many open file descriptors.
> Looks leaky.");
> 57: }
This test case may be not reliable if there are some other test cases or
applications running at the same time. It's a good manual test, but might be
not suitable for OpenJDK automation regression test if it could be impacted.
test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 37:
> 35: * will not leave leaking socket file descriptors
> 36: * @library /test/lib
> 37: * @run main/othervm SSLSocketLeak
See bellow comment, I may suggest to have it as a manual test case if you agree
the test case could be impacted.
@run main/manual SSLSocketLeak
-
PR: https://git.openjdk.java.net/jdk/pull/1363
Re: RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources
On Fri, 27 Nov 2020 15:46:08 GMT, Christoph Langer wrote: >> I changed the check for when to do duplexClose to only do it when socket >> isConnected(). >> >> I also added a testcase which should work on all platforms. For windows I >> borrowed some functionality introduced lately with test >> java/lang/ProcessBuilder/checkHandles/CheckHandles.java which I moved to the >> test library for that reason. >> >> Now it's ready to review. > > Ping... Any takers? comments? reviews? I will have a look at this update. - PR: https://git.openjdk.java.net/jdk/pull/1363
