[8u] RFR 8209333: Socket reset issue for TLS 1.3 socket close

[Alvarez, David]

Hi,

I have uploaded two new versions of the patch.

The first one replaces the try-with-resources with its equivalent. This
removes the need for the SuppressWarnings:

http://cr.openjdk.java.net/~alvdavi/webrevs/8209333/webrev.8u.jdk.01/


The second one is what Andrew was suggesting, just adding the finally
block. The only problem with this approach is that an exception in the
close call would override an exception in the deplete call. The deplete
call seems pretty safe though, as it is already suppresing potential
IOExceptions, so this is probably fine.

http://cr.openjdk.java.net/~alvdavi/webrevs/8209333/webrev.8u.jdk.02/

I really don't care which version gets approved. I went through all
three before. My first implementation was v02, then I moved to v01
because I wanted to ensure there was no possible difference on how
exceptions were being thrown and finally to v00 with the
SuppressWarnings because v01 was just ugly. 

I hope this helps. Just make clear which version gets approved so I can
mention it in the JBS bug when adding the jdk8u-fix-request.

Thanks,

David


On Wed, 2021-01-13 at 15:02 +0100, Volker Simonis wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> Andrew Haley <aph at redhat.com> schrieb am Mi., 13. Jan. 2021, 11:30:
> 
> > On 1/12/21 5:46 PM, Volker Simonis wrote:
> > > Notice that it is perfectly fine to put an autoclosable resources
> > > in
> > > the try clause, even if that resource is not used within the try
> > > block
> > > itself. After all, that's the reason why Java 9 simplified this
> > > pattern by extending "try-with resources" to also accept already
> > > existing resources as final or "effectively final" variables in
> > > the
> > > try statement [1]. Nothing obscure at all.
> > 
> > One person's obscure is another's apogee of clarity, I suppose.
> > The problem here, IMO, is that the TWR construct doesn't do
> > anything useful: it's only hiding a catch-then-close.
> > 
> > > In 8 however, we have to declare a new variable for the resource
> > > in
> > > the try clause, because the Java 9 simplification isn't
> > > available. If
> > > we were to write new code for jdk8 I could agree with you to use
> > > the
> > > older, more traditional pattern. But we're reviewing a downport
> > > here
> > > and for downports we have other priorities:
> > >  - keep modifications of a change to a minimum
> > >  - try to not impact future downports
> > 
> > OK, so for you deviations from upstream patches are more important
> > than idiomatic JDK8 code. I get that, but in this case the
> > resulting
> > code is such that it provokes a warning, which I expect you'll
> > admit
> > is something of a red flag. Any backports to a "stable" release
> > that
> > generate new warnings are a big deal, IMO, although I grant you
> > that
> > this warning is not about anything terribly important.
> > 
> 
> But this warning is suppress by an annotation as it is already done
> in
> several other places in the 8u code base.
> 
> If that's still not acceptable for you I won't argue any further and
> recommend David to rewrite his downport as proposed by you. In the
> end we
> only want to fix the underlaying issue.
> 
> 
> > > As David mentioned, more backports in this area will follow
> > > that's why
> > > I think that the code version he presented is the best
> > > compromise.
> > 
> > In this exact area, as in these lines of code or the ones next to
> > them? If not, that doesn't matter.
> > 
> > --
> > Andrew Haley  (he/him)
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > https://keybase.io/andrewhaley
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
> > 
> >