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

[Lindenmaier, Goetz]

Hi Martin,

Thanks for looking at this. 

> 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.
Yes, I think this is a question for the original change. 

> 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.
I tried to document well that I did some edits to the change for 
This file. This bug is linked to the orginal [2]. So if someone downports
[2] he should be able to find out about this.
Taking the whole change to 11 seems too risky to me.

Best regards,
  Goetz.


> 
> 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.u
> diff.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
> >