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

[Gary Adams]

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

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