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 <gary.ad...@oracle.com>
> Sent: Donnerstag, 20. Juni 2019 11:56
> To: Langer, Christoph <christoph.lan...@sap.com>
> Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>;
> Schmelter, Ralf <ralf.schmel...@sap.com>
> 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 <gary.ad...@oracle.com>
> >> Sent: Mittwoch, 19. Juni 2019 16:32
> >> To: Langer, Christoph <christoph.lan...@sap.com>
> >> Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>;
> >> Schmelter, Ralf <ralf.schmel...@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.ad...@oracle.com>
> >>>> Sent: Mittwoch, 19. Juni 2019 13:59
> >>>> To: Langer, Christoph<christoph.lan...@sap.com>
> >>>> Cc: OpenJDK Serviceability<serviceability-dev@openjdk.java.net>;
> >>>> Schmelter, Ralf<ralf.schmel...@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-
> >> boun...@openjdk.java.net>
> >>>> On
> >>>>>> Behalf Of gary.ad...@oracle.com
> >>>>>> Sent: Dienstag, 18. Juni 2019 12:08
> >>>>>> To: OpenJDK Serviceability<serviceability-dev@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;
> >>>>>>> + }
> >>>>>>> }
> >>>>>>> }
