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

[Chris Plummer]

ok

On 6/19/19 9:49 AM, Gary Adams wrote:
> It is not a new flag.
> It is the socket fd.
> This aligns with the hPipe used in the windows impl.
>
> On 6/19/19, 12:46 PM, Chris Plummer wrote:
>> 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;
>>>>>>>> +            }
>>>>>>>>            }
>>>>>>>>        }
>>>
>>
>