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

[Volker Simonis]

Hi everybody,

I think that David's downport is correct and there's nothing much we
can do about the warning except suppressing it.

"try-with resources" was introduced in Java 7 and allowed the
declaration of resources in the try clause [1].

Later, in Java 9, "try-with resources" was extended to also accept
already existing resources as final or "effectively final" variables
in the try statement [2].

The initial change uses such an "effectively final" resource (i.e.
"conContext.inputRecord"):

try (conContext.inputRecord) {
    appInput.deplete();
}

Unfortunately that exact version can't be downported as is because
that syntax is simply not supported in Java 8. So we have to declare a
new variable in the try statement as David did:

try (InputRecord ir = conContext.inputRecord) {
    appInput.deplete();
}

But now javac complains that "ir" is not referenced in body of
corresponding try statement (which is true, but we know that it exists
and we know that it must be closed if an exception occurs). The
"SuppressWarnings("try")" annotation exist for this exact scenario.
Notice that this annotations is already used in the jdk8
class-library, e.g. in BufferedWriter.java [3] or
FilterOutputStream.java[4] in similar situations. In Java 9 and later,
javac doesn't emits this warning for final or "effectively final"
variables which are declared in the try statement but not used in the
try body.

Using Andrew H's proposal would certainly work as well, but from my
point of view it would be a bigger difference compared to the original
change so I'd vote for keeping David's version.

Thank you and best regards,
Volker

[1] https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
[2] https://docs.oracle.com/en/java/javase/15/language/java-language-changes.html#GUID-A920DB06-0FD1-4F9C-8A9A-15FC979D5DA3
[3] https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7432d3558458/src/share/classes/java/io/BufferedWriter.java#l258
[4] https://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/7432d3558458/src/share/classes/java/io/FilterOutputStream.java#l155

On Mon, Jan 11, 2021 at 11:28 AM Andrew Haley <aph at redhat.com> wrote:
>
> On 1/11/21 4:08 AM, Andrew Hughes wrote:
> > On 21:01 Fri 08 Jan     , Alvarez, David wrote:
> >> Hi,
> >>
> >> I would like a review for 8209333. This is one of the Socket related
> >> patches that were applied to OpenJDK11 after the backport of TLS 1.3 to
> >> OpenJDK8
> >>
> >> BugId: https://bugs.openjdk.java.net/browse/JDK-8209333
> >> Webrev:
> >> http://cr.openjdk.java.net/~alvdavi/webrevs/8209333/webrev.8u.jdk.00/
> >>
> >> The only relevant differences are in SSLSocketImpl.java, as it
> >> contained a java 9 try-with-resources block [1]. A
> >> @SuppressWarnings("try") was needed.
> >
> > What warning was emitted? Is it not possible to rewrite this so
> > as to avoid the warning?
>
> Yes, I think this warning is for a real bug. Should be something like:
>
>             if (!conContext.isInboundClosed()) {
>                 try {
>                     // Try the best to use up the input records and close the
>                     // socket gracefully, without impact the performance too
>                     // much.
>                     appInput.deplete();
>                 } finally {
>                     conContext.inputRecord.close();
>                 }
>             }
>
> --
> 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
>