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

[Langer, Christoph]

Hi Gary,

this is better. The detach method already uses synchronization in each platform implementation.

I think this improved close behavior should be implemented in all platform implementations of VirtualMachineImpl. That is aix, linux, macosx, solaris and windows. For Windows, it's the PipedInputStream::close method (line 173) which should also have the better implementation.

As for fix target: I think you should push it to JDK13 still - it is a P3 bug which is within criteria for RDP1.

Thanks
Christoph

> -----Original Message-----
> From: Gary Adams <gary.adams at oracle.com>
> Sent: Mittwoch, 19. Juni 2019 16:32
> 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
>
> That would be consistent with the windows detach() synchronization.
>
> Updated patch is attached.
>
> On 6/19/19, 8:14 AM, Langer, Christoph wrote:
> > 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;
> >>>>> +            }
> >>>>>            }
> >>>>>        }