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

[Chris Plummer]

Hi Gary,

Is there a reason you've chosen an int flag rather than a boolean? Other 
than that the changes look fine.

thanks,

Chris

On 6/19/19 7:31 AM, Gary Adams wrote:
> 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;
>>>>>> +            }
>>>>>>            }
>>>>>>        }
>