11u: RFR[M]: 8240827: Downport SSLSocketImpl.java from "8221882: Use fiber-friendly java.util.concurrent.locks in JSSE"

[Martin Balao]

Hi Goetz,

Thanks for doing this work.

I'm a bit late here but will share my comments anyways (note: I'm not an
official reviewer).

Backported patches look good to me. My only question in that regard is
why do we need a call to "readLockedDeplete" from the "read" method here
[1]. Looks to me at first sight that "readLockedDeplete" will be called
anyways by "deplete" [2]; and if "deplete" were not called, then
"isClosing" will be false and the call in [1] won't be performed
anyways. Perhaps there is something obvious I'm not seeing. CC'ing
Xuelei for this just in case.

In regards to the backport patch, I'm not sure about decision taken on
OutputRecord's lock. My concern is that when we backport 8221882 we
forget about this and we end-up synchronizing against something
different in  SSLSocketImpl.java. Is it too much trouble to propagate
the changes to OutputRecord? I had a first look but you probably have
analyzed that better.

Thanks,
Martin.-

--
[1] - http://hg.openjdk.java.net/jdk/jdk/rev/ca251ef47e0b#l1.75
[2] - http://hg.openjdk.java.net/jdk/jdk/rev/ca251ef47e0b#l1.87


On 3/10/20 1:01 PM, Lindenmaier, Goetz wrote:
> Hi,
> 
> I started downporting [1], "8209333: Socket reset issue for TLS 1.3 socket close"
> for parity with 11.0.8-oracle.
> 
> Severin pointed me to [3], "8219991: New fix of the deadlock in
> sun.security.ssl.SSLSocketImpl", a follow-up that fixes an issue
> introduced by [1]. I agree that it would make sense
> to bring this fix to jdk11u-dev along with [1].
> 
> [1] applies clean to jdk11u-dev.
> Unfortunately, [3] does not apply at all.
> 
> In jdk13 a major rework of JSSE was done:
> [2] "8221882: Use fiber-friendly java.util.concurrent.locks in JSSE"
> 
> [2] removes the synchronized keywords from a lot of functions
> in JSSE and replaces them by manual locking. [3] exploits
> the new control flow within these reworked functions in
> SSLSocketImpl.java. It needs to be designed differently
> to apply directly on top of [1].
> 
> Instead, I propose to downport the changes of [2] to the only
> file touched by [3], SSLSocketImpl.java.
> 
> I took the patch of [2] for SSLSocketImpl.java and patched
> it on top of [1] to jdk11u-dev. It applies clean. On top
> of this, [3] applies clean, too.
> 
> The SSLSocketImpl.java part of [2] uses a public lock
> introduced in OutputRecord. I revoked this part of the change
> as the changes to OutputRecord are not important here, so
> it'd like to skip them. Here the partial webrev of the
> revoked code:
> http://cr.openjdk.java.net/~goetz/wr20/8240827-Downport_SSLSocketImpl_from_8221882-jdk11/01-revoked/src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java.udiff.html
> 
> The locks introduced by [2] in SSLSocketImpl.java are private,
> so there is no direct dependency to them in other code.
> 
> I ran the three changes through our nightly testing
> and saw no issues.
> 
> Please review:
> http://cr.openjdk.java.net/~goetz/wr20/8240827-Downport_SSLSocketImpl_from_8221882-jdk11/01/
> 
> Best regards,
>   Goetz.
> 
> [1] "8209333: Socket reset issue for TLS 1.3 socket close"
> https://bugs.openjdk.java.net/browse/JDK-8209333
> http://hg.openjdk.java.net/jdk/jdk12/rev/8a61a04c456c
> 
> [2] "8221882: Use fiber-friendly java.util.concurrent.locks in JSSE"
> https://bugs.openjdk.java.net/browse/JDK-8221882
> http://hg.openjdk.java.net/jdk/jdk/rev/dfba4e321ab3
> 
> [3] "8219991: New fix of the deadlock in sun.security.ssl.SSLSocketImpl"
> https://bugs.openjdk.java.net/browse/JDK-8219991
> http://hg.openjdk.java.net/jdk/jdk/rev/ca251ef47e0b
>