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

[Gary Adams]

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 8224642.patch
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190619/db6a961a/8224642.patch>