RE: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Ok, then jdk/jdk is fine for me. In case this turns out to be more important, I guess it could be backported to an update release. > -Original Message- > From: gary.ad...@oracle.com > Sent: Donnerstag, 20. Juni 2019 11:56 > To: Langer, Christoph > Cc: OpenJDK Serviceability ; > Schmelter, Ralf > Subject: Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java > fails: Bad file descriptor > > 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 > >> Sent: Mittwoch, 19. Juni 2019 16:32 > >> To: Langer, Christoph > >> Cc: OpenJDK Serviceability ; > >> Schmelter, Ralf > >> 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 > >>>> Sent: Mittwoch, 19. Juni 2019 13:59 > >>>> To: Langer, Christoph > >>>> Cc: OpenJDK Serviceability; > >>>> Schmelter, Ralf > >>>> 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 >> boun...@openjdk.java.net> > >>>> On > >>>>>> Behalf Of gary.ad...@oracle.com > >>>>>> Sent: Dienstag, 18. Juni 2019 12:08 > >>>>>> To: OpenJDK Serviceability > >>>>>> Subject:
Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 Sent: Mittwoch, 19. Juni 2019 16:32 To: Langer, Christoph Cc: OpenJDK Serviceability ; Schmelter, Ralf 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 Sent: Mittwoch, 19. Juni 2019 13:59 To: Langer, Christoph Cc: OpenJDK Serviceability; Schmelter, Ralf 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 boun...@openjdk.java.net> On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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
RE: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 > Sent: Mittwoch, 19. Juni 2019 16:32 > To: Langer, Christoph > Cc: OpenJDK Serviceability ; > Schmelter, Ralf > 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 > >> Sent: Mittwoch, 19. Juni 2019 13:59 > >> To: Langer, Christoph > >> Cc: OpenJDK Serviceability; > >> Schmelter, Ralf > >> 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 boun...@openjdk.java.net> > >> On > >>>> Behalf Of gary.ad...@oracle.com > >>>> Sent: Dienstag, 18. Juni 2019 12:08 > >>>> To: OpenJDK Serviceability > >>>> 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]; > >&
Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 Sent: Mittwoch, 19. Juni 2019 13:59 To: Langer, Christoph Cc: OpenJDK Serviceability; Schmelter, Ralf 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 On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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; + } } }
Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 Sent: Mittwoch, 19. Juni 2019 13:59 To: Langer, Christoph Cc: OpenJDK Serviceability; Schmelter, Ralf 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 On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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; +} } }
Re: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 Sent: Mittwoch, 19. Juni 2019 13:59 To: Langer, Christoph Cc: OpenJDK Serviceability; Schmelter, Ralf 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 On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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; + } } }
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 Sent: Mittwoch, 19. Juni 2019 13:59 To: Langer, Christoph Cc: OpenJDK Serviceability; Schmelter, Ralf 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 On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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; +} } } # HG changeset patch # User gadams # Date 1560954512 14400 # Wed Jun 19 10:28:32 2019 -0400 # Node ID 40b15f2605afc70922041cf544aae3559ad53068 # Parent e9da3a44a7edc0222e727b83bd758b9e43176cee 8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor Reviewed-by: cjplummer, rschmelter, clanger 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/
RE: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 > Sent: Mittwoch, 19. Juni 2019 13:59 > To: Langer, Christoph > Cc: OpenJDK Serviceability ; > Schmelter, Ralf > 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 > On > >> Behalf Of gary.ad...@oracle.com > >> Sent: Dienstag, 18. Juni 2019 12:08 > >> To: OpenJDK Serviceability > >> 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; > >>> +} > >>> } > >>> }
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 On Behalf Of gary.ad...@oracle.com Sent: Dienstag, 18. Juni 2019 12:08 To: OpenJDK Serviceability 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; +} } } # HG changeset patch # User gadams # Date 1560945060 14400 # Wed Jun 19 07:51:00 2019 -0400 # Node ID ff79eb44347a5aed75ddd8d536ffcf784384c126 # Parent e0be41293b41975cd8e20c5d63ad0e176e12df3c 8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor Reviewed-by: cjplummer, rschmelter, clanger 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 { -VirtualMachineImpl.close(s); +if (s != -1) { +VirtualMachineImpl.close(s); +s = -1; +} } }
RE: RFR: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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 On > Behalf Of gary.ad...@oracle.com > Sent: Dienstag, 18. Juni 2019 12:08 > To: OpenJDK Serviceability > 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; > > + } > > } > > }
RE: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Hi, looking at the source code, all Unix variants use the same unsafe code in the close method of the returned InputStream. On Windows there is already a check, but it is not synchronized. Best regards, Ralf
RE: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Hi, > So you prefer the fix in VirtualMachineImpl SocketInputStream close() > rather than the JCmd try with resources. Yes, since the close() method is not spec conform. I think it is better to fix it than to adjust the code using it. Especially if you consider that the current behavior could be used to 'steal' a file descriptor. Best regards, Ralf
Re: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
So you prefer the fix in VirtualMachineImpl SocketInputStream close() rather than the JCmd try with resources. If we were dealing with output streams, it would be important to apply the flush to the writer. Since these are input streams we just need to guard against the duplicate close for the native resource. On 6/18/19 6:34 AM, Schmelter, Ralf wrote: Hi, since InputStream imeplements Closeable, calling close multiple times *must* work: public interface Closeable extends AutoCloseable { /** * Closes this stream and releases any system resources associated * with it. If the stream is already closed then invoking this * method has no effect. * * As noted in {@link AutoCloseable#close()}, cases where the * close may fail require careful attention. It is strongly advised * to relinquish the underlying resources and to internally * mark the {@code Closeable} as closed, prior to throwing * the {@code IOException}. * * @throws IOException if an I/O error occurs */ public void close() throws IOException; } So the close() method must be fixed in the same way FileInputStream and friends implement close(). Best regards, Ralf
RE: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Hi, since InputStream imeplements Closeable, calling close multiple times *must* work: public interface Closeable extends AutoCloseable { /** * Closes this stream and releases any system resources associated * with it. If the stream is already closed then invoking this * method has no effect. * * As noted in {@link AutoCloseable#close()}, cases where the * close may fail require careful attention. It is strongly advised * to relinquish the underlying resources and to internally * mark the {@code Closeable} as closed, prior to throwing * the {@code IOException}. * * @throws IOException if an I/O error occurs */ public void close() throws IOException; } So the close() method must be fixed in the same way FileInputStream and friends implement close(). Best regards, Ralf
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; + } } }
Re: JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
Yeah, I think if your double close check works, then the proper fix really should be to rework the original implementation of JDK-8222491 so no double close is done. Chris On 6/17/19 12: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; + } } }
JDK-8224642: Test sun/tools/jcmd/TestJcmdSanity.java fails: Bad file descriptor
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; +} } }