Re: RFR: 8284796: sun.security.ssl.Finished::toString misses a line feed in the message format pattern
On Thu, 14 Apr 2022 18:08:21 GMT, Xue-Lei Andrew Fan wrote:
>> The log for Finished message looks like the below,
>>
>> "Finished": {
>> "verify data": {
>> : ... ...
>> }'} // looks weird
>>
>> because a line feed is missing in the format pattern.
>>
>> ""Finished": '{'\n" +
>> " "verify data": '{'\n" +
>> "{0}\n" +
>> " '}'" + // a line feed is needed
>> "'}'",
>
> Marked as reviewed by xuelei (Reviewer).
@XueleiFan Thanks for your approval!
-
PR: https://git.openjdk.java.net/jdk/pull/8215
Integrated: 8284796: sun.security.ssl.Finished::toString misses a line feed in the message format pattern
On Wed, 13 Apr 2022 05:10:22 GMT, John Jiang wrote:
> The log for Finished message looks like the below,
>
> "Finished": {
> "verify data": {
> : ... ...
> }'} // looks weird
>
> because a line feed is missing in the format pattern.
>
> ""Finished": '{'\n" +
> " "verify data": '{'\n" +
> "{0}\n" +
> " '}'" + // a line feed is needed
> "'}'",
This pull request has now been integrated.
Changeset: d9708206
Author:John Jiang
URL:
https://git.openjdk.java.net/jdk/commit/d9708206164a0b7bfe611b597b49c5e75c37ad47
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
8284796: sun.security.ssl.Finished::toString misses a line feed in the message
format pattern
Reviewed-by: xuelei
-
PR: https://git.openjdk.java.net/jdk/pull/8215
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 13 Apr 2022 22:04:03 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update w/ review feedbacks
CSR is filed at: https://bugs.openjdk.java.net/browse/JDK-8284897
Please review, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v2]
> https://bugs.openjdk.java.net/browse/JDK-8284688 > > [JDK-8273046](https://bugs.openjdk.java.net/browse/JDK-8273046) is the > umbrella bug for this bug. The changes were too large for a single code > review, so it was decided to split into smaller chunks. This is one such > chunk: > > open/src/java.security.jgss/share/classes/javax/security > open/src/java.security.jgss/share/classes/org/ietf > open/src/java.security.jgss/share/classes/sun/security Mark Powers has updated the pull request incrementally with one additional commit since the last revision: fourth iteration - Changes: - all: https://git.openjdk.java.net/jdk/pull/7746/files - new: https://git.openjdk.java.net/jdk/pull/7746/files/fb0b7f16..e4292aef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7746=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7746=00-01 Stats: 63 lines in 10 files changed: 13 ins; 0 del; 50 mod Patch: https://git.openjdk.java.net/jdk/pull/7746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7746/head:pull/7746 PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v2]
On Thu, 14 Apr 2022 22:01:55 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> use static cleaner, and clean up swp file
Thanks for updating this.
Can a test case be included?
While it doesn't _seem_ like `this` would need to be captured for these
lambdas, it's good to be sure -- now, and in the future.
If test code could access a PKCS11 instance, the WeakHashMap technique from [PR
8136](https://github.com/openjdk/jdk/pull/8136#issuecomment-1092881171) could
be used. The same goes for P11KeyStore (well, a `PasswordCallbackHandler`
instance, in that case).
But I don't know how practical it would be for test code to get access to those
objects.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v2]
On Thu, 14 Apr 2022 19:26:53 GMT, Daniel Fuchs wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> use static cleaner, and clean up swp file > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java > line 168: > >> 166: // Calls disconnect() to cleanup the native part of the wrapper. >> 167: Cleaner.create().register(this, >> 168: () -> PKCS11.disconnect(pNativeData)); > > If I'm not mistaken each new call to Cleaner.create() will create a new > cleaner and a new deamon thread for it, so it's not recommended to create one > cleaner per object. > Also it might be worth double checking that the lambda created here doesn't > capture `this`: IIRC there were some subtle cases where a lambda could > unexpectedly capture `this`. > > Also probably the cleaner itself can't be GC'ed while its thread is running > but you might be relying on undocumented behavior. It would be more prudent > to stick it in a static variable to retain a strong reference. Good points. I may change to use a static method instead in the next commit. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v2]
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
Xue-Lei Andrew Fan has updated the pull request incrementally with one
additional commit since the last revision:
use static cleaner, and clean up swp file
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8248/files
- new: https://git.openjdk.java.net/jdk/pull/8248/files/898154b9..d9f4e00d
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8248=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8248=00-01
Stats: 16 lines in 8 files changed: 5 ins; 2 del; 9 mod
Patch: https://git.openjdk.java.net/jdk/pull/8248.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8248/head:pull/8248
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
On Thu, 14 Apr 2022 19:22:00 GMT, Valerie Peng wrote: > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/.PKCS11.java.swp > file should not be there? Oops, I missed this swp file while using git commit. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 19:56:22 GMT, Bradford Wetmore wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add `@LastModified: Apr 2022` to DocumentCache > > src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMXPathFilter2Transform.java > line 110: > >> 108: int length = attributes.getLength(); >> 109: Map namespaceMap = >> 110: HashMap.newHashMap(length); > > Please check these changes with @seanjmullan before integrating. @seanjmullan mirror pr in https://github.com/apache/santuario-xml-security-java/pull/64 jira at https://issues.apache.org/jira/projects/SANTUARIO/issues/SANTUARIO-586?filter=reportedbyme - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v22]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: java.xml.crypto's usage downgrade grammar to 1.8 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/71b7dba3..95e22f25 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=21 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=20-21 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Thu, 14 Apr 2022 21:05:06 GMT, Daniel Jeliński wrote: >> During TLS handshake, hundreds of constraints are evaluated to determine >> which cipher suites are usable. Most of the evaluations are performed using >> `HandshakeContext#algorithmConstraints` object. By default that object >> contains a `SSLAlgorithmConstraints` instance wrapping another >> `SSLAlgorithmConstraints` instance. As a result the constraints defined in >> `SSLAlgorithmConstraints` are evaluated twice. >> >> This PR improves the default case; if the user-specified constraints are >> left at defaults, we use a single `SSLAlgorithmConstraints` instance, and >> avoid duplicate checks. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - add more factory methods, update copyright > - return DEFAULT also when user constraints are null I think I addressed all the concerns raised. Could you take another look? - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
> During TLS handshake, hundreds of constraints are evaluated to determine > which cipher suites are usable. Most of the evaluations are performed using > `HandshakeContext#algorithmConstraints` object. By default that object > contains a `SSLAlgorithmConstraints` instance wrapping another > `SSLAlgorithmConstraints` instance. As a result the constraints defined in > `SSLAlgorithmConstraints` are evaluated twice. > > This PR improves the default case; if the user-specified constraints are left > at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid > duplicate checks. Daniel Jeliński has updated the pull request incrementally with two additional commits since the last revision: - add more factory methods, update copyright - return DEFAULT also when user constraints are null - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/b6e46ae9..e4cc8152 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8199=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8199=01-02 Stats: 65 lines in 6 files changed: 33 ins; 2 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 18:32:03 GMT, Naoto Sato wrote: >>> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 >> >> @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But >> usually we only solve 1 issue in 1 pr, so I think it's better to wait after >> this. > > Thanks. Will fix it myself, as I want to eliminate that immediate value in > the code. PR opened, based on this PR. https://github.com/openjdk/jdk/pull/8253 - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 19:53:45 GMT, Bradford Wetmore wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add `@LastModified: Apr 2022` to DocumentCache > > I learned something new about HashMap today... > > I looked at java.security.cert and sun.security.* and that part LGTM. > > That said, you need to check with @seanjmullan for the java.xml.crypto code. > We try to keep the code in sync with the Apache code. As this is a new API, > we probably can't push this kind of change to Apache as they need to support > older releases. Thanks @bradfordwetmore and @seanjmullan for looking at this, and @XenoAmess for following up quickly. To summarize, it sounds like the only issues are with the changes to two files in the `java.xml.crypto` area, as those need to be maintained in sync with Apache Santuario. Right? In both cases it looks like the HashMap is likely being under-allocated, so the fix would be to inline to capacity computation, something like `new HashMap<>((int) Math.ceil(length / 0.75))` I guess. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. I checked over: java.base/macosx/classes/apple/security java.base/share/classes/com/sun/crypto java.base/share/classes/com/sun/security java.base/share/classes/java/security java.base/share/classes/javax/crypto java.base/share/classes/javax/net java.base/share/classes/sun/security The copyright dates need updating. src/java.base/share/classes/sun/security/provider/certpath/AdjacencyList.java line 128: > 126: // Each time this method is called, we're examining a new list > 127: // from the global list. So, we have to start by getting the list > 128: // that contains the set of Vertices we're considering. The class being affected is a Vertex, so either change to vertices, or leave as is... - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 20:11:37 GMT, XenoAmess wrote: > > Are the changes necessary for this part? > > @seanjmullan no, they are just performance refinement. > > If you really that wanna 100% sync , > > I can use the old 1.8 api to migrate that part, and make a mirror pr to that > part of https://github.com/apache/santuario-xml-security-java > > Is this solution acceptable then? Yes, that would be preferred. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 18:10:28 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add `@LastModified: Apr 2022` to DocumentCache > Are the changes necessary for this part? @seanjmullan no, they are just performance refinement. If you really that wanna 100% sync , I can use the old 1.8 api to migrate that part, and make a mirror pr to that part of https://github.com/apache/santuario-xml-security-java Is this solution acceptable then? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Looks good. I usually like GitHub's colorful diffs, but this is one of those rare cases where looking at the single webrev-generated diff file is so much easier. About the only thing that could improve it would be to reduce the context (i.e. "diff -C"). - Marked as reviewed by iris (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 18:10:28 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add `@LastModified: Apr 2022` to DocumentCache I learned something new about HashMap today... I looked at java.security.cert and sun.security.* and that part LGTM. That said, you need to check with @seanjmullan for the java.xml.crypto code. We try to keep the code in sync with the Apache code. As this is a new API, we probably can't push this kind of change to Apache as they need to support older releases. src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMXPathFilter2Transform.java line 110: > 108: int length = attributes.getLength(); > 109: Map namespaceMap = > 110: HashMap.newHashMap(length); Please check these changes with @seanjmullan before integrating. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 18:10:28 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add `@LastModified: Apr 2022` to DocumentCache Right, we generally try to avoid making too many changes to the implementation code in the java.xml.crypto module in order to stay consistent with Apache Santuario. They also would not accept this change because it is a new API and they need to run on older releases. I haven't had time yet to understand this Enhancement, but are the changes necessary for this part? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. src/java.base/share/classes/jdk/internal/icu/impl/NormalizerImpl.java line 2002: > 2000: } > 2001: > 2002: // this is where we are right now with all these > indices: Although these are actual typos, they come from upstream ICU code. Changing them locally would make merging complicated, so please exclude ICU related changes from the PR. I guess fixes in other 3rd party libraries are in the same boat. - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v2]
On Wed, 13 Apr 2022 19:27:00 GMT, Valerie Peng wrote:
>> This trivial change is to deprecate the DEFAULT static field of
>> OAEPParameterSpec class. Wordings are mostly the same as the previous
>> PSSParameterSpec deprecation change. Rest are just minor code re-factoring.
>>
>> The CSR will be filed once review is somewhat finished.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update w/ review feedback.
src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 113:
> 111:
> 112: // disallowed
> 113: private OAEPParameterSpec() {
I think you can just remove this ctor now that it is not used by `DEFAULT`.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
On Thu, 14 Apr 2022 18:06:10 GMT, Xue-Lei Andrew Fan wrote:
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java
line 168:
> 166: // Calls disconnect() to cleanup the native part of the wrapper.
> 167: Cleaner.create().register(this,
> 168: () -> PKCS11.disconnect(pNativeData));
If I'm not mistaken each new call to Cleaner.create() will create a new cleaner
and a new deamon thread for it, so it's not recommended to create one cleaner
per object.
Also it might be worth double checking that the lambda created here doesn't
capture `this`: IIRC there were some subtle cases where a lambda could
unexpectedly capture `this`.
Also probably the cleaner itself can't be GC'ed while its thread is running but
you might be relying on undocumented behavior. It would be more prudent to
stick it in a static variable to retain a strong reference.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
RFR: 8284893: Fix typos in java.base
I ran `codespell` on the `src/java.base` directory, and accepted those changes where it indeed discovered real typos. (Due to false positives this can unfortunately not be run automatically) The majority of fixes are in comments. A handful is in strings, one in a local variable name, and a couple in parameter declarations. Annoyingly, there are several instances of "childs" (instead of "children") in the source code, but they were not local and I dared not change them. Someone braver than me might take a stab at it, perhaps.. - Commit messages: - Pass #2 - 8284893: Fix typos in java.base Changes: https://git.openjdk.java.net/jdk/pull/8250/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8250=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284893 Stats: 268 lines in 180 files changed: 0 ins; 0 del; 268 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
On Thu, 14 Apr 2022 18:06:10 GMT, Xue-Lei Andrew Fan wrote:
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/.PKCS11.java.swp
file should not be there?
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 17:06:53 GMT, XenoAmess wrote: >> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 > >> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 > > @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But usually > we only solve 1 issue in 1 pr, so I think it's better to wait after this. Thanks. Will fix it myself, as I want to eliminate that immediate value in the code. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 18:10:28 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add `@LastModified: Apr 2022` to DocumentCache Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v20]
On Thu, 14 Apr 2022 18:05:48 GMT, XenoAmess wrote: >> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java >> line 3: >> >>> 1: /* >>> 2: * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights >>> reserved. >>> 3: */ >> >> The LastModified tag was missing in this class. Pls use this opportunity to >> add it in the same format as the other classes (CoreDocumentImpl, >> XSAttributeChecker), that is after line 52. Thanks. > > @JoeWang-Java done. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
This is an effort to fix a problem introduced in the fix for
[JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which replaced
the finalizers in jdk.crypto.cryptoki with Cleaners. However, there is a
problem with the code changes. The Runnables registered with Cleaner refer to
the object being registered ('this'). Meaning, the Cleaner mechanism will keep
the objects reachable, preventing them from being cleaned and collected.
-
Commit messages:
- 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
Changes: https://git.openjdk.java.net/jdk/pull/8248/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8248=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8284855
Stats: 46 lines in 6 files changed: 10 ins; 21 del; 15 mod
Patch: https://git.openjdk.java.net/jdk/pull/8248.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8248/head:pull/8248
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284796: sun.security.ssl.Finished::toString misses a line feed in the message format pattern
On Wed, 13 Apr 2022 05:10:22 GMT, John Jiang wrote:
> The log for Finished message looks like the below,
>
> "Finished": {
> "verify data": {
> : ... ...
> }'} // looks weird
>
> because a line feed is missing in the format pattern.
>
> ""Finished": '{'\n" +
> " "verify data": '{'\n" +
> "{0}\n" +
> " '}'" + // a line feed is needed
> "'}'",
Marked as reviewed by xuelei (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8215
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: add `@LastModified: Apr 2022` to DocumentCache - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/5b437dab..71b7dba3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=20 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=19-20 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v20]
On Thu, 14 Apr 2022 17:23:42 GMT, Joe Wang wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes on ProcessEnvironment > > src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java > line 3: > >> 1: /* >> 2: * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights >> reserved. >> 3: */ > > The LastModified tag was missing in this class. Pls use this opportunity to > add it in the same format as the other classes (CoreDocumentImpl, > XSAttributeChecker), that is after line 52. Thanks. @JoeWang-Java done. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 10:38:33 GMT, Yi Yang wrote: >I found [yet another >typo](https://github.com/kelthuzadx/jdk/commit/acb9e15bc0bf5395d1c0875f36992f692734f948), > I wonder if you can merge this into your patch so that I do not need to >submit a new PR for it? Thanks. I think it deserves a separate ticket. BTW there are a lot of other typos in JDK, especially in comments. - PR: https://git.openjdk.java.net/jdk/pull/8231
Integrated: 8284853: Fix various 'expected' typo
On Wed, 13 Apr 2022 20:36:48 GMT, Andrey Turbanov wrote: > Found various typos of expected: `exepected`, `exept`, `epectedly`, > `expeced`, `Unexpeted`, etc. This pull request has now been integrated. Changeset: 48c75498 Author:Andrey Turbanov URL: https://git.openjdk.java.net/jdk/commit/48c75498060f076287d3d44c49934db9ac70887b Stats: 65 lines in 28 files changed: 0 ins; 0 del; 65 mod 8284853: Fix various 'expected' typo Reviewed-by: bpb, ihse - PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v20]
On Thu, 14 Apr 2022 17:05:39 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes on ProcessEnvironment src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java line 3: > 1: /* > 2: * Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights > reserved. > 3: */ The LastModified tag was missing in this class. Pls use this opportunity to add it in the same format as the other classes (CoreDocumentImpl, XSAttributeChecker), that is after line 52. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:53:15 GMT, Naoto Sato wrote: > Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But usually we only solve 1 issue in 1 pr, so I think it's better to wait after this. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v20]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: revert changes on ProcessEnvironment - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/5603f193..5b437dab Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=19 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=18-19 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 23:25:47 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update LastModified
>
> src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
>
>> 100: /* Only for use by Runtime.exec(...String[]envp...) */
>> 101: static Map emptyEnvironment(int capacity) {
>> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
>
> This change is correct, since this is called with the length of an array
> that's used to populate the environment. It really should be named `size`
> instead of `capacity`. However the windows version of this code simply calls
> `super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
> probably not worth changing this now. We may need to revisit this later to do
> some cleaning up. (And also the strange computation in the static initializer
> at line 71.)
@stuart-marks OK, changes on this class reverted.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v19]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: fix usage in XSAttributeChecker - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/d110ecfd..5603f193 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=18 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=17-18 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 03:38:52 GMT, Joe Wang wrote: >>> I suspect the `size*2+1` was a failed attempt at allocating a HashMap of >>> the correct capacity for `size` mappings. >> >> I looked the codes and don't think so.. >> If I'm wrong, I'm glad to fix. > > Stuart's right, I looked at the code, it's as you said a failed attempt, > "size" would be good. So HashMap.newHashMap(size) would actually be a small > improvement. > > It's an interesting impl the way it used HashMap, but it's 20 year code. @JoeWang-Java @stuart-marks got it. done. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 15:53:53 GMT, Xue-Lei Andrew Fan wrote: >> as of today, this method is never called with a `null` argument >> (`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to >> `DEFAULT` and cannot be reset to `null`), but I can add a null check for >> future-proofing. > > I know. But if the null condition is not added, a code reader may have to > search for its usage and make sure null is not passed. If the usages are in > the same class, I may not add the checking. Otherwise, an additional > checking might save time in the future. In such cases `assert xxx != null;` could be used to tell the reader that `null` is not an expected value. But then you need to be absolutely sure that `null` can never reach here when in production. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 15:43:42 GMT, Daniel Jeliński wrote: >>> @XueleiFan did you mean `||` (not `&&`) ? >> >> Thank you @dfuch. Yes, it should be "||". > > as of today, this method is never called with a `null` argument > (`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to > `DEFAULT` and cannot be reset to `null`), but I can add a null check for > future-proofing. I know. But if the null condition is not added, a code reader may have to search for its usage and make sure null is not passed. If the usages are in the same class, I may not add the checking. Otherwise, an additional checking might save time in the future. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 14:58:24 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 73:
>>
>>> 71:
>>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>>> userSpecifiedConstraints) {
>>> 73: if (userSpecifiedConstraints == DEFAULT) {
>>
>> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>>
>>
>> -if (userSpecifiedConstraints == DEFAULT) {
>> +if (userSpecifiedConstraints == null &&
>> + userSpecifiedConstraints== DEFAULT) {
>
>> @XueleiFan did you mean `||` (not `&&`) ?
>
> Thank you @dfuch. Yes, it should be "||".
as of today, this method is never called with a `null` argument
(`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to
`DEFAULT` and cannot be reset to `null`), but I can add a null check for
future-proofing.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with two > additional commits since the last revision: > > - typo blank linke correction > - revise the update IMO we should not send close_notify in the finalizer. It's the application's responsibility to send close_notify when it's done with the socket; we should not pretend that it was closed normally when it was not. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284796: sun.security.ssl.Finished::toString misses a line feed in the message format pattern
On Wed, 13 Apr 2022 07:14:04 GMT, John Jiang wrote: > Could I not change this style? Sure. But effort of this update will be overrode if text blocks are used. I would suggest to use text block once the code block get touched. - PR: https://git.openjdk.java.net/jdk/pull/8215
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 04:24:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid nesting SSLAlgorithmConstraints
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 73:
>
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>> 73: if (userSpecifiedConstraints == DEFAULT) {
>
> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>
>
> -if (userSpecifiedConstraints == DEFAULT) {
> +if (userSpecifiedConstraints == null &&
> + userSpecifiedConstraints== DEFAULT) {
> @XueleiFan did you mean `||` (not `&&`) ?
Thank you @dfuch. Yes, it should be "||".
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 09:28:17 GMT, Andrey Turbanov wrote: >> Found various typos of expected: `exepected`, `exept`, `epectedly`, >> `expeced`, `Unexpeted`, etc. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8284853: Fix various 'expected' typo > improve test log Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 09:28:17 GMT, Andrey Turbanov wrote: >> Found various typos of expected: `exepected`, `exept`, `epectedly`, >> `expeced`, `Unexpeted`, etc. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8284853: Fix various 'expected' typo > improve test log I found [yet another typo](https://github.com/kelthuzadx/jdk/commit/acb9e15bc0bf5395d1c0875f36992f692734f948), I wonder if you can merge this into your patch so that I do not need to submit a new PR for it? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284853: Fix various 'expected' typo [v2]
> Found various typos of expected: `exepected`, `exept`, `epectedly`, > `expeced`, `Unexpeted`, etc. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8284853: Fix various 'expected' typo improve test log - Changes: - all: https://git.openjdk.java.net/jdk/pull/8231/files - new: https://git.openjdk.java.net/jdk/pull/8231/files/fe6d9890..9fc75e89 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8231=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8231=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8231/head:pull/8231 PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 04:24:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid nesting SSLAlgorithmConstraints
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 73:
>
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>> 73: if (userSpecifiedConstraints == DEFAULT) {
>
> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>
>
> -if (userSpecifiedConstraints == DEFAULT) {
> +if (userSpecifiedConstraints == null &&
> + userSpecifiedConstraints== DEFAULT) {
@XueleiFan did you mean `||` (not `&&`) ?
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 19:28:08 GMT, Stuart Marks wrote: > Reviewers for i18n, net, nio, and security, please review call site changes > in your areas. Thanks. Changes to `java.net.http` look good to me. I haven't spotted anything obviously wrong in the rest, but should defer to reviewers of these areas. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote:
>> Please review the update to remove finalizer method in the SunJSSE provider
>> implementation. It is one of the efforts to clean up the use of finalizer
>> method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - typo blank linke correction
> - revise the update
I parsed the finalize() code one more time. The sending close_notify may be
not an expected part of the finalize() implementation.
The finalize() calls the close() method. If the socket is layered over a
preexisting socket, the preexisting socket is closed by calling it close()
method:
`self.close();
`
Otherwise, the SSLSocket.close() method will be called:
`super.close();
`
See the BaseSSLSocketImpl.close() method:
@Override
public void close() throws IOException {
if (self == this) {
super.close();
} else {
self.close();
}
}
For layered over socket case, if the preexisting socket is not an SSLSocket
object(which is the common case), no close_notify will be sent off course. If
the preexisting socket is an SSLSocket object (which may be not common), the
SSLSocketImpl.close() will be called and the close_notify could be sent.
For non-layered over sockets, as super.close() is called, there is no
close_notify delivered during the finalizing.
Based on the cases above, the close_notify delivery may be not an expected
behavior during finalization in practice. I would like to remove the
finalize() method without adding a cleaner, as shown in the current PR. But
if you read the code and behavior differently , it's acceptable to me to clean
up the preexisting socket, by adding a cleaner like:
BaseSSLSocketImpl(Socket socket) {
super();
this.self = socket;
this.consumedInput = null;
+ CleanerFactory.cleaner().register(this, self::close);
}
Please let me know your preference.
-
PR: https://git.openjdk.java.net/jdk/pull/8065
