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

[Gary Adams]

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;
>>>>>>> +            }
>>>>>>>            }
>>>>>>>        }
>>
>