Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: restore JSR166; restore java.desktop - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/903f0305..a9e7dbc8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=09-10 Stats: 49 lines in 7 files changed: 24 ins; 7 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v8]
On Wed, 7 Jul 2021 17:08:19 GMT, Mandy Chung wrote: >>> Does "client changes" means changes involving src/java.desktop and >>> test/java/awt? >> >> src/java.desktop, test/java/awt, and test/javax/imageio > >> > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java >> > needs to be updated in JSR 166 upstream repo. Better to file a separate >> > issue for this change to ensure that gets fixed in the upstream project. >> >> Can you please paste a link for that? I'm not sure where I can find JSR 166 >> upstream repo.. > > What I meant is to file a JBS issue for this change and revert the change in > this patch. That can be fixed when the next JSR 166 changes are imported to > JDK. > > I wasn't sure if this is the right repo: > http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/ Okay. I've filed https://bugs.openjdk.java.net/browse/JDK-8270057 and https://bugs.openjdk.java.net/browse/JDK-8270058 for JSR 166 and client code respectively. These codes have been restored. (Sorry for force-pushing, my mistake..) - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server [v2]
On Wed, 7 Jul 2021 17:22:07 GMT, Xue-Lei Andrew Fan wrote:
>> Alexey Bakhtin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add read lock for inputRecord.deplete
>
> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1789:
>
>> 1787: appInput.readLock.unlock();
>> 1788: }
>> 1789: }
>
> The blocking on close() may be not good in practice. I would use tryLock()
> rather than lock() so as to avoid closure blocking. tryLock() is not
> perfect, but it may be better than blocking the close().
>
> BTW, you could use the intanceof pattern matching so as to avoid the cast
> (See https://openjdk.java.net/jeps/394).
>
>
> if (conContext.inputRecord instanceof
> SSLSocketInputRecord inputRecord &&
> isConnected) {
> if (appInput.readLock.tryLock()) {
> try {
> inputRecord.deplete(false);
> } finally {
>appInput.readLock.unlock();
> }
> }
> }
Hi Xuelei,
Thank you a lot again. Replaced lock with tryLock as you suggested
-
PR: https://git.openjdk.java.net/jdk/pull/4520
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server [v3]
> Please review the fix for JDK-8268965. > > The new jtreg test is added for the described issue. > sun/security/ssl and javax/net/ssl tests are passed Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision: Use tryLock - Changes: - all: https://git.openjdk.java.net/jdk/pull/4520/files - new: https://git.openjdk.java.net/jdk/pull/4520/files/198d0d96..dc51d9ef Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4520=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4520=01-02 Stats: 8 lines in 1 file changed: 1 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4520.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4520/head:pull/4520 PR: https://git.openjdk.java.net/jdk/pull/4520
Re: RFR: 8269933: test/jdk/javax/net/ssl/compatibility/JdkInfo incorrect verification of protocol and cipher support
On Wed, 7 Jul 2021 15:23:09 GMT, Fernando Guallini
wrote:
> test/jdk/javax/net/ssl/compatibility/JdkInfo is a helper class for the
> compatibility tests. It is verifying whether a protocol or cipher suite is
> supported/enabled by setting all the allowed values as a string, and then
> invoking String contains() to return whether a specific version is supported.
> This approach is problematic when, for instance, supportedProtocols is equal
> to 'TLSv1.3,TLSv1.2', then supportedProtocols.contains("TLSv1") will return
> true, given that 'TLSv1' is effectively a substring of 'TLSv1.3'.
>
> Proposed fix: Set the supported/enabled protocols and ciphers as elements in
> lists, and use List contains() to find matches
Marked as reviewed by rhalade (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4710
Re: RFR: 8269933: test/jdk/javax/net/ssl/compatibility/JdkInfo incorrect verification of protocol and cipher support
On Wed, 7 Jul 2021 15:23:09 GMT, Fernando Guallini
wrote:
> test/jdk/javax/net/ssl/compatibility/JdkInfo is a helper class for the
> compatibility tests. It is verifying whether a protocol or cipher suite is
> supported/enabled by setting all the allowed values as a string, and then
> invoking String contains() to return whether a specific version is supported.
> This approach is problematic when, for instance, supportedProtocols is equal
> to 'TLSv1.3,TLSv1.2', then supportedProtocols.contains("TLSv1") will return
> true, given that 'TLSv1' is effectively a substring of 'TLSv1.3'.
>
> Proposed fix: Set the supported/enabled protocols and ciphers as elements in
> lists, and use List contains() to find matches
Marked as reviewed by xuelei (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4710
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server [v2]
On Wed, 7 Jul 2021 10:15:19 GMT, Alexey Bakhtin wrote:
>> Please review the fix for JDK-8268965.
>>
>> The new jtreg test is added for the described issue.
>> sun/security/ssl and javax/net/ssl tests are passed
>
> Alexey Bakhtin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add read lock for inputRecord.deplete
src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1789:
> 1787: appInput.readLock.unlock();
> 1788: }
> 1789: }
The blocking on close() may be not good in practice. I would use tryLock()
rather than lock() so as to avoid closure blocking. tryLock() is not perfect,
but it may be better than blocking the close().
BTW, you could use the intanceof pattern matching so as to avoid the cast (See
https://openjdk.java.net/jeps/394).
if (conContext.inputRecord instanceof
SSLSocketInputRecord inputRecord &&
isConnected) {
if (appInput.readLock.tryLock()) {
try {
inputRecord.deplete(false);
} finally {
appInput.readLock.unlock();
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/4520
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Wed, 7 Jul 2021 16:22:25 GMT, Mandy Chung wrote: >> Hi Mandy, thanks for reviewing this. >> >>> I suggest to separate the client changes (both src and test) in a separate >>> PR for the client review. >> >> Does "client changes" means changes involving src/java.desktop and >> test/java/awt? >> >>> src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java >>> needs to be updated in JSR 166 upstream repo. Better to file a separate >>> issue for this change to ensure that gets fixed in the upstream project. >> >> Can you please paste a link for that? I'm not sure where I can find JSR 166 >> upstream repo.. >> >>> Nit: The above formatting (line 70-97) is inconsistent with the formatting >>> in line 110-124. It'd be good to use the same formatting. >> >> Restored. > >> Does "client changes" means changes involving src/java.desktop and >> test/java/awt? > > src/java.desktop, test/java/awt, and test/javax/imageio > > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java > > needs to be updated in JSR 166 upstream repo. Better to file a separate > > issue for this change to ensure that gets fixed in the upstream project. > > Can you please paste a link for that? I'm not sure where I can find JSR 166 > upstream repo.. What I meant is to file a JBS issue for this change and revert the change in this patch. That can be fixed when the next JSR 166 changes are imported to JDK. I wasn't sure if this is the right repo: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/ - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]
On Wed, 7 Jul 2021 02:10:10 GMT, Yi Yang wrote: > Does "client changes" means changes involving src/java.desktop and > test/java/awt? src/java.desktop, test/java/awt, and test/javax/imageio - PR: https://git.openjdk.java.net/jdk/pull/4507
RFR: 8269933: test/jdk/javax/net/ssl/compatibility/JdkInfo incorrect verification of protocol and cipher support
test/jdk/javax/net/ssl/compatibility/JdkInfo is a helper class for the
compatibility tests. It is verifying whether a protocol or cipher suite is
supported/enabled by setting all the allowed values as a string, and then
invoking String contains() to return whether a specific version is supported.
This approach is problematic when, for instance, supportedProtocols is equal to
'TLSv1.3,TLSv1.2', then supportedProtocols.contains("TLSv1") will return true,
given that 'TLSv1' is effectively a substring of 'TLSv1.3'.
Proposed fix: Set the supported/enabled protocols and ciphers as elements in
lists, and use List contains() to find matches
-
Commit messages:
- refactored JdkInfo
Changes: https://git.openjdk.java.net/jdk/pull/4710/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4710=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8269933
Stats: 27 lines in 1 file changed: 15 ins; 0 del; 12 mod
Patch: https://git.openjdk.java.net/jdk/pull/4710.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4710/head:pull/4710
PR: https://git.openjdk.java.net/jdk/pull/4710
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server [v2]
On Wed, 7 Jul 2021 10:15:19 GMT, Alexey Bakhtin wrote: >> Please review the fix for JDK-8268965. >> >> The new jtreg test is added for the described issue. >> sun/security/ssl and javax/net/ssl tests are passed > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Add read lock for inputRecord.deplete Hi Xuelei, Thank you for review. You are right. It is better to add readLock to exclude concurrent reading. Unfortunately I can not use appInput.deplete() here because of input stream already marked as closed (called from TransportContext.fatal(TransportContext.java:376)). So, I've added appInput.readLock around deplete() - PR: https://git.openjdk.java.net/jdk/pull/4520
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server [v2]
> Please review the fix for JDK-8268965. > > The new jtreg test is added for the described issue. > sun/security/ssl and javax/net/ssl tests are passed Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision: Add read lock for inputRecord.deplete - Changes: - all: https://git.openjdk.java.net/jdk/pull/4520/files - new: https://git.openjdk.java.net/jdk/pull/4520/files/cebf52aa..198d0d96 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4520=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4520=00-01 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4520.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4520/head:pull/4520 PR: https://git.openjdk.java.net/jdk/pull/4520
