RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor

[Langer, Christoph]

Hi Gary,

looks good overall. I however think the block should also be synchronized to avoid issues when multiple threads attempt to close the stream.

Cheers
Christoph

> -----Original Message-----
> From: Gary Adams <gary.adams at oracle.com>
> Sent: Mittwoch, 19. Juni 2019 13:59
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: OpenJDK Serviceability <serviceability-dev at openjdk.java.net>;
> Schmelter, Ralf <ralf.schmelter at sap.com>
> Subject: Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java
> fails: Bad file descriptor
> 
> I think everyone is in agreement now that preventing the double close
> is the best way to handle this failure. If there are no further comments,
> I'll push the attached patch on Thurs morning to the jdk/jdk repos.
> 
> I'll also close JDK-8223361 as a duplicate.
> 
> On 6/19/19, 2:36 AM, Langer, Christoph wrote:
> > Hi Gary,
> >
> > I think overall it would be better to fix the InputStream to be tolerant to
> multiple calls to close, as Ralf pointed out. Maybe someone else on some
> other place runs into this again because he/she relies on the correct
> implementation of Closeable.
> >
> > However, as a quick fix I can also imagine to do use a single resource like
> this:
> >
> > try (InputStreamReader isr = new
> InputStreamReader(hvm.executeJCmd(line), "UTF-8")) {
> >
> > Then we'd also have a single close call per instance.
> >
> > But if you do that, you should at least open a bug to track fixing of the
> InputStream implementation...
> >
> > Best regards
> > Christoph
> >
> >> -----Original Message-----
> >> From: serviceability-dev<serviceability-dev-bounces at openjdk.java.net>
> On
> >> Behalf Of gary.adams at oracle.com
> >> Sent: Dienstag, 18. Juni 2019 12:08
> >> To: OpenJDK Serviceability<serviceability-dev at openjdk.java.net>
> >> Subject: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails:
> >> Bad file descriptor
> >>
> >> The workaround below passed 1000 testruns on linux-x64-debug.
> >>
> >> A more localized fix simply moves the stream reader out of the
> >> try with resources, so only one close is applied to the underlying
> >> socket. I'll run this test through 1000 testruns today.
> >>
> >> Looking for a final review for this change.
> >>
> >> diff --git a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
> >> b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
> >> --- a/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
> >> +++ b/src/jdk.jcmd/share/classes/sun/tools/jcmd/JCmd.java
> >> @@ -122,8 +122,8 @@
> >>                if (line.trim().equals("stop")) {
> >>                    break;
> >>                }
> >> -            try (InputStream in = hvm.executeJCmd(line);
> >> -                 InputStreamReader isr = new InputStreamReader(in,
> >> "UTF-8")) {
> >> +            try (InputStream in = hvm.executeJCmd(line)) {
> >> +                InputStreamReader isr = new InputStreamReader(in, "UTF-8");
> >>                    // read to EOF and just print output
> >>                    char c[] = new char[256];
> >>                    int n;
> >>
> >> On 6/17/19 3:23 PM, Gary Adams wrote:
> >>> https://bugs.openjdk.java.net/browse/JDK-8224642
> >>>
> >>> I may have a handle on what is going wrong with the
> >>> TestJcmdSanity test and the bad file descriptor.
> >>>
> >>> A change made in April 2019 placed the input stream and reader
> >>> within the same try with resources block. This has the effect of
> >>> calling the
> >>> SocketInputStream close method twice for each command processed.
> >>>
> >>> https://bugs.openjdk.java.net/browse/JDK-8222491
> >>>    http://hg.openjdk.java.net/jdk/jdk/rev/4224f26b2e7f
> >>>
> >>> The last set of tests in the TestJcmdSanity test attempts to process ~100
> >>> VM.version commands in a loop. Since the closes are handled
> >>> when the objects are collected it may come at an inopportune time.
> >>>
> >>> I'm testing the fix below to ensure a second close becomes a noop.
> >>> It may be better to revisit the earlier change that set up the double
> >>> close calls.
> >>>
> >>> diff --git
> >>> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> >>>
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> >>> ---
> >>> a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> >>> +++
> >>>
> b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
> >>> @@ -233,7 +233,7 @@
> >>>        * InputStream for the socket connection to get target VM
> >>>        */
> >>>       private class SocketInputStream extends InputStream {
> >>> -        int s;
> >>> +        int s = -1;
> >>>
> >>>           public SocketInputStream(int s) {
> >>>               this.s = s;
> >>> @@ -261,7 +261,10 @@
> >>>           }
> >>>
> >>>           public void close() throws IOException {
> >>> +            if (s != -1) {
> >>>               VirtualMachineImpl.close(s);
> >>> +                s = -1;
> >>> +            }
> >>>           }
> >>>       }