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