Re: [PATCH] remove redundant initialization of volatile fields with default values
Cool, thanks! Do you know anyone who could sponsor this and create a web-review against the patch? Regards, Sergeyt Tsypanov 13.08.2020, 19:22, "Sean Mullan" : > On 8/13/20 9:04 AM, Сергей Цыпанов wrote: >> Hi, >> >> I don't have account in JBS, so I cannot file an issue. >> >> Previously when I submitted patches via core-libs-dev mailing list >> previleged users >> filed the issues and created web-reviews. >> >> I think this should be a subtask of >> https://bugs.openjdk.java.net/browse/JDK-6736490, there's >> already one I've mentioned in previous mail: >> https://bugs.openjdk.java.net/browse/JDK-8145680 > > Done: see https://bugs.openjdk.java.net/browse/JDK-8251548 > > --Sean > >> Regards, >> Sergey Tsypanov >> >> 13.08.2020, 14:05, "Sean Mullan" : >>> On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHboPdHMd6$ It appears that there are no constraitns and Doug Lea mentions in https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHbvX4nrL2$ that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. >>> >>> Ok, it seems like a good change. Are you able to file a JBS issue for >>> this? After that you can request a formal code review. >>> >>> Thanks, >>> Sean
Re: RFR JDK-8250839: Improve test template SSLEngineTemplate with SSLContextTemplate
All good catches! I will update accordingly. Thanks, Xuelei On 8/13/2020 11:13 AM, Anthony Scarpino wrote: On 8/11/20 9:44 AM, Xuelei Fan wrote: ping ... On 7/30/2020 11:26 AM, Xuelei Fan wrote: Hi, May I get the following test code update reviewed? http://cr.openjdk.java.net/~xuelei/8250839/webrev.00/ SSLEngineTemplate is a template used for SSLEngine testing, which depends on binary key store files, and not easy to extend. This update makes it easier to extend, by removing the files dependency and using SSLContextTemplate. Thanks, Xuelei Just a few nits 109-114: Did you want these commented out? I have no problem leaving it as is. I just wanted to make sure this was your intent. 219: The whitespaces are not aligned Otherwise it looks fine. No need for another webrev. Tony
Re: RFR JDK-8250839: Improve test template SSLEngineTemplate with SSLContextTemplate
On 8/11/20 9:44 AM, Xuelei Fan wrote: ping ... On 7/30/2020 11:26 AM, Xuelei Fan wrote: Hi, May I get the following test code update reviewed? http://cr.openjdk.java.net/~xuelei/8250839/webrev.00/ SSLEngineTemplate is a template used for SSLEngine testing, which depends on binary key store files, and not easy to extend. This update makes it easier to extend, by removing the files dependency and using SSLContextTemplate. Thanks, Xuelei Just a few nits 109-114: Did you want these commented out? I have no problem leaving it as is. I just wanted to make sure this was your intent. 219: The whitespaces are not aligned Otherwise it looks fine. No need for another webrev. Tony
Re: Fix for Javadoc errors in java.base
On 8/13/20 1:21 PM, Jonathan Gibbons wrote: --- old/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:21.233726447 +0530 +++ new/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:20.721720857 +0530 @@ -96,8 +96,6 @@ * @param p the prime modulus * @param g the base generator * @param l the private-value length - * - * @exception InvalidKeyException if the key cannot be encoded This should actually remain, but it should be ProviderException which is a RuntimeException. See the other DHPrivateKey ctor as that specifies it correctly. --Sean I note the use of `@exception`, as compared to `@throws`, which is more common. Stats: `@exception` 7322 occurrences `@throws` 21173 occurrences That's probably too many `@exception` to clean up. :-( Right, that's probably a separate cleanup activity. However, if you want to change the 3 instances of @exception to @throws in DHPrivateKey, I'm fine with that. --Sean
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 8/13/20 9:04 AM, Сергей Цыпанов wrote: Hi, I don't have account in JBS, so I cannot file an issue. Previously when I submitted patches via core-libs-dev mailing list previleged users filed the issues and created web-reviews. I think this should be a subtask of https://bugs.openjdk.java.net/browse/JDK-6736490, there's already one I've mentioned in previous mail: https://bugs.openjdk.java.net/browse/JDK-8145680 Done: see https://bugs.openjdk.java.net/browse/JDK-8251548 --Sean Regards, Sergey Tsypanov 13.08.2020, 14:05, "Sean Mullan" : On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHboPdHMd6$ It appears that there are no constraitns and Doug Lea mentions in https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHbvX4nrL2$ that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. Ok, it seems like a good change. Are you able to file a JBS issue for this? After that you can request a formal code review. Thanks, Sean
Re: Fix for Javadoc errors in java.base
On 8/13/20 10:13 AM, Sean Mullan wrote: On 8/13/20 11:05 AM, Julia Boes wrote: Hi Vipin, Thanks for providing this fix, I'm happy to sponsor your change. To complete the review, we still need someone to green light the remaining changes below. I'm looping in net-dev and security-dev to have a look. Bug: https://bugs.openjdk.java.net/browse/JDK-8251542 Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8251542/webrev/ Once the review is completed, please provide me with a patch that includes a changeset comment as described here: https://openjdk.java.net/guide/producingChangeset.html If you have any questions, let me know. Cheers, Julia --- old/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:21.233726447 +0530 +++ new/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:20.721720857 +0530 @@ -96,8 +96,6 @@ * @param p the prime modulus * @param g the base generator * @param l the private-value length - * - * @exception InvalidKeyException if the key cannot be encoded This should actually remain, but it should be ProviderException which is a RuntimeException. See the other DHPrivateKey ctor as that specifies it correctly. --Sean I note the use of `@exception`, as compared to `@throws`, which is more common. Stats: `@exception` 7322 occurrences `@throws` 21173 occurrences That's probably too many `@exception` to clean up. :-( -- Jon
Re: Fix for Javadoc errors in java.base
On 8/13/20 11:05 AM, Julia Boes wrote: Hi Vipin, Thanks for providing this fix, I'm happy to sponsor your change. To complete the review, we still need someone to green light the remaining changes below. I'm looping in net-dev and security-dev to have a look. Bug: https://bugs.openjdk.java.net/browse/JDK-8251542 Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8251542/webrev/ Once the review is completed, please provide me with a patch that includes a changeset comment as described here: https://openjdk.java.net/guide/producingChangeset.html If you have any questions, let me know. Cheers, Julia --- old/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:21.233726447 +0530 +++ new/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java 2020-07-25 23:46:20.721720857 +0530 @@ -96,8 +96,6 @@ * @param p the prime modulus * @param g the base generator * @param l the private-value length - * - * @exception InvalidKeyException if the key cannot be encoded This should actually remain, but it should be ProviderException which is a RuntimeException. See the other DHPrivateKey ctor as that specifies it correctly. --Sean
Re: Fix for Javadoc errors in java.base
Hi Vipin, Julia, On 13/08/2020 16:05, Julia Boes wrote: Hi Vipin, Thanks for providing this fix, I'm happy to sponsor your change. To complete the review, we still need someone to green light the remaining changes below. I'm looping in net-dev and security-dev to have a look. Changes to Socket and ServerSocket look fine. No CSR needed since this is a package private API. Bug: https://bugs.openjdk.java.net/browse/JDK-8251542 Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8251542/webrev/ best regards, -- daniel
Re: Fix for Javadoc errors in java.base
Hi Vipin,
Thanks for providing this fix, I'm happy to sponsor your change. To
complete the review, we still need someone to green light the remaining
changes below. I'm looping in net-dev and security-dev to have a look.
Bug: https://bugs.openjdk.java.net/browse/JDK-8251542
Webrev: http://cr.openjdk.java.net/~jboes/webrevs/8251542/webrev/
Once the review is completed, please provide me with a patch that
includes a changeset comment as described here:
https://openjdk.java.net/guide/producingChangeset.html
If you have any questions, let me know.
Cheers,
Julia
--- old/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java
2020-07-25 23:46:21.233726447 +0530
+++ new/src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java
2020-07-25 23:46:20.721720857 +0530
@@ -96,8 +96,6 @@
* @param p the prime modulus
* @param g the base generator
* @param l the private-value length
- *
- * @exception InvalidKeyException if the key cannot be encoded
*/
DHPrivateKey(BigInteger x, BigInteger p, BigInteger g, int l) {
this.x = x;
---
old/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
2020-07-25 23:46:22.241737383 +0530
+++
new/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
2020-07-25 23:46:21.745732013 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 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
@@ -208,7 +208,7 @@
*
* @return a CallSite, which, when invoked, will return an instance of the
* functional interface
- * @throws ReflectiveOperationException
+ * @throws LambdaConversionException
*/
abstract CallSite buildCallSite()
throws LambdaConversionException;
---
old/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
2020-07-25 23:46:23.261748361 +0530
+++
new/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
2020-07-25 23:46:22.761742991 +0530
@@ -200,7 +200,6 @@
*
* @return a CallSite, which, when invoked, will return an instance of the
* functional interface
- * @throws ReflectiveOperationException
* @throws LambdaConversionException If properly formed functional
interface
* is not found
*/
--- old/src/java.base/share/classes/java/net/ServerSocket.java 2020-07-25
23:46:26.449782093 +0530
+++ new/src/java.base/share/classes/java/net/ServerSocket.java 2020-07-25
23:46:25.937776742 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1995, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 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
@@ -315,7 +315,7 @@
/**
* Creates the socket implementation.
*
- * @throws IOException if creation fails
+ * @throws SocketException if creation fails
* @since 1.4
*/
void createImpl() throws SocketException {
--- old/src/java.base/share/classes/java/net/Socket.java 2020-07-25
23:46:27.485792869 +0530
+++ new/src/java.base/share/classes/java/net/Socket.java 2020-07-25
23:46:26.973787565 +0530
@@ -533,7 +533,7 @@
*
* @param stream a {@code boolean} value : {@code true} for a TCP socket,
* {@code false} for UDP.
- * @throws IOException if creation fails
+ * @throws SocketException if creation fails
* @since 1.4
*/
void createImpl(boolean stream) throws SocketException {
Re: [PATCH] remove redundant initialization of volatile fields with default values
Hi, I don't have account in JBS, so I cannot file an issue. Previously when I submitted patches via core-libs-dev mailing list previleged users filed the issues and created web-reviews. I think this should be a subtask of https://bugs.openjdk.java.net/browse/JDK-6736490, there's already one I've mentioned in previous mail: https://bugs.openjdk.java.net/browse/JDK-8145680 Regards, Sergey Tsypanov 13.08.2020, 14:05, "Sean Mullan" : > On 8/13/20 7:04 AM, Сергей Цыпанов wrote: >> Hello, >> >> previously I've sent an email regarding removal of redundant assignments if >> default values to volatile fields, see >> https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html >> >> There was a concern whether it's completely safe to remove those >> assignments from JMM point of view, see >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html >> >> Recently I've found a thread in concurrency-interest mailing list where >> Aleksey Shiplive tried to find a constraint >> agians such removal: >> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html >> >> It appears that there are no constraitns and Doug Lea mentions in >> >> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html >> that "there is never any reason to explicitly initialize fields to >> 0/0.0/false/null" >> >> Also there we similar code changes in java.base before: >> >> - https://bugs.openjdk.java.net/browse/JDK-6736490 >> - https://bugs.openjdk.java.net/browse/JDK-8035284 >> - https://bugs.openjdk.java.net/browse/JDK-8145680 >> >> So I think now we can accept the patch as the changes appear to be safe. > > Ok, it seems like a good change. Are you able to file a JBS issue for > this? After that you can request a formal code review. > > Thanks, > Sean
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html It appears that there are no constraitns and Doug Lea mentions in http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. Ok, it seems like a good change. Are you able to file a JBS issue for this? After that you can request a formal code review. Thanks, Sean
[PATCH] remove redundant initialization of volatile fields with default values
Hello,
previously I've sent an email regarding removal of redundant assignments if
default values to volatile fields, see
https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html
There was a concern whether it's completely safe to remove those assignments
from JMM point of view, see
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html
Recently I've found a thread in concurrency-interest mailing list where Aleksey
Shiplive tried to find a constraint
agians such removal:
http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html
It appears that there are no constraitns and Doug Lea mentions in
http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html
that "there is never any reason to explicitly initialize fields to
0/0.0/false/null"
Also there we similar code changes in java.base before:
- https://bugs.openjdk.java.net/browse/JDK-6736490
- https://bugs.openjdk.java.net/browse/JDK-8035284
- https://bugs.openjdk.java.net/browse/JDK-8145680
So I think now we can accept the patch as the changes appear to be safe.
Best regards,
Sergey Tsypanovdiff --git a/src/java.base/share/classes/java/security/KeyStore.java b/src/java.base/share/classes/java/security/KeyStore.java
--- a/src/java.base/share/classes/java/security/KeyStore.java
+++ b/src/java.base/share/classes/java/security/KeyStore.java
@@ -219,7 +219,7 @@
private KeyStoreSpi keyStoreSpi;
// Has this keystore been initialized (loaded)?
-private boolean initialized = false;
+private boolean initialized;
/**
* A marker interface for {@code KeyStore}
@@ -264,7 +264,7 @@
private final char[] password;
private final String protectionAlgorithm;
private final AlgorithmParameterSpec protectionParameters;
-private volatile boolean destroyed = false;
+private volatile boolean destroyed;
/**
* Creates a password parameter.
diff --git a/src/java.base/share/classes/java/util/ListResourceBundle.java b/src/java.base/share/classes/java/util/ListResourceBundle.java
--- a/src/java.base/share/classes/java/util/ListResourceBundle.java
+++ b/src/java.base/share/classes/java/util/ListResourceBundle.java
@@ -206,5 +206,5 @@
lookup = temp;
}
-private volatile Map lookup = null;
+private volatile Map lookup;
}
diff --git a/src/java.base/share/classes/javax/security/auth/Subject.java b/src/java.base/share/classes/javax/security/auth/Subject.java
--- a/src/java.base/share/classes/javax/security/auth/Subject.java
+++ b/src/java.base/share/classes/javax/security/auth/Subject.java
@@ -126,7 +126,7 @@
*
* @serial
*/
-private volatile boolean readOnly = false;
+private volatile boolean readOnly;
private static final int PRINCIPAL_SET = 1;
private static final int PUB_CREDENTIAL_SET = 2;
diff --git a/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java b/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
--- a/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
+++ b/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
@@ -71,7 +71,7 @@
// Common working status
-private boolean instantiated = false;
+private boolean instantiated;
/**
* Reseed counter of a DRBG instance. A mechanism should increment it
@@ -80,7 +80,7 @@
*
* Volatile, will be used in a double checked locking.
*/
-protected volatile int reseedCounter = 0;
+protected volatile int reseedCounter;
// Mech features. If not same as below, must be redefined in constructor.
diff --git a/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java b/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
--- a/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
+++ b/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
@@ -36,7 +36,7 @@
*/
final class DTLSOutputRecord extends OutputRecord implements DTLSRecord {
-private DTLSFragmenter fragmenter = null;
+private DTLSFragmenter fragmenter;
int writeEpoch;
@@ -44,7 +44,7 @@
Authenticator prevWriteAuthenticator;
SSLWriteCipher prevWriteCipher;
-private volatile boolean isCloseWaiting = false;
+private volatile boolean isCloseWaiting;
DTLSOutputRecord(HandshakeHash handshakeHash) {
super(handshakeHash, SSLWriteCipher.nullDTlsWriteCipher());
diff --git a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
--- a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
+++ b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
@@ -102,11 +102,11 @@
boolean isResumption;
SSLSessionImpl resumingSession;
// Session
