RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
gary.adams at oracle.com
gary.adams at oracle.com
Thu Jun 20 09:56:06 UTC 2019
During testing the failure was only observed in a questionable
test on linux-x64-debug builds. I question whether P3 was a
correct assessment when the bug was filed. The only reason
this encounter caused a problem with the double close is the test
was looping and getting the same file descriptor and the second close
came while new socket was being allocated.
I have no issue with delivering this fix into jdk/jdk,
but if it is needed in jdk13, I'll have to hand it off to
someone else to complete.
On 6/19/19 5:56 PM, Langer, Christoph wrote:
> Hi Gary,
>
> this is better. The detach method already uses synchronization in each platform implementation.
>
> I think this improved close behavior should be implemented in all platform implementations of VirtualMachineImpl. That is aix, linux, macosx, solaris and windows. For Windows, it's the PipedInputStream::close method (line 173) which should also have the better implementation.
>
> As for fix target: I think you should push it to JDK13 still - it is a P3 bug which is within criteria for RDP1.
>
> Thanks
> Christoph
>
>> -----Original Message-----
>> From: Gary Adams <gary.adams at oracle.com>
>> Sent: Mittwoch, 19. Juni 2019 16:32
>> 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
>>
>> 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;
>>>>>>> + }
>>>>>>> }
>>>>>>> }
More information about the serviceability-dev
mailing list