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