Re: Fwd: Patch: Clean up fix for JDK-8047720
On 4/02/2015 1:28 PM, Carsten Varming wrote: Dear David Holmes, Please see inline response, On Tue, Feb 3, 2015 at 9:38 PM, David Holmes mailto:david.hol...@oracle.com>> wrote: On 4/02/2015 5:00 AM, Carsten Varming wrote: Greetings all, I was recently introduced to the deadlock described in JDK-8047720 and fixed in JDK9. The fix seems a little messy to me, and it looks like it left some very short races in the code. So I decided to clean up the code. See attached diff. The change takes a step back and acquires PeriodicTask_lock in WatcherThread::stop (like before the fix in JDK9), but this time safepoints are allowed to proceed when acquiring PeriodicTask_lock, preventing the deadlock. It isn't obvious that blocking for a safepoint whilst in this method will always be a safe thing to do. That would need to be examined in detail - potential interactions can be very subtle. Absolutely true. For reference, the pattern is repeated with the Terminator_lock a few lines below. The pattern is also used in Threads::destroy_vm before and after calling before_exit, and the java shutdown hooks are called right before the call to before_exit. So there is a lot of evidence that safepoints are allowed to happen in this context. The thread calling before_exit is a JavaThread so of course it participates in safepoints. The issue is whether the interaction between that thread and the WatcherThread, via the PeriodicTask_lock, can allow for the JavaThread blocking at a safepoint whilst owning that lock. If another JavaThread can try to lock it without a safepoint check then we can get a deadlock. Cheers, David Thank you for taking your time to look at this, Carsten David Let me know what you think, Carsten
Re: Fwd: Patch: Clean up fix for JDK-8047720
Dear David Holmes, Please see inline response, On Tue, Feb 3, 2015 at 9:38 PM, David Holmes wrote: > On 4/02/2015 5:00 AM, Carsten Varming wrote: > >> Greetings all, >> >> I was recently introduced to the deadlock described in JDK-8047720 and >> fixed in JDK9. The fix seems a little messy to me, and it looks like it >> left some very short races in the code. So I decided to clean up the >> code. See attached diff. >> >> The change takes a step back and acquires PeriodicTask_lock in >> WatcherThread::stop (like before the fix in JDK9), but this time >> safepoints are allowed to proceed when acquiring PeriodicTask_lock, >> preventing the deadlock. >> > > It isn't obvious that blocking for a safepoint whilst in this method will > always be a safe thing to do. That would need to be examined in detail - > potential interactions can be very subtle. Absolutely true. For reference, the pattern is repeated with the Terminator_lock a few lines below. The pattern is also used in Threads::destroy_vm before and after calling before_exit, and the java shutdown hooks are called right before the call to before_exit. So there is a lot of evidence that safepoints are allowed to happen in this context. Thank you for taking your time to look at this, Carsten > > David > > > Let me know what you think, >> Carsten >> >>
Re: Fwd: Patch: Clean up fix for JDK-8047720
On 4/02/2015 5:00 AM, Carsten Varming wrote: Greetings all, I was recently introduced to the deadlock described in JDK-8047720 and fixed in JDK9. The fix seems a little messy to me, and it looks like it left some very short races in the code. So I decided to clean up the code. See attached diff. The change takes a step back and acquires PeriodicTask_lock in WatcherThread::stop (like before the fix in JDK9), but this time safepoints are allowed to proceed when acquiring PeriodicTask_lock, preventing the deadlock. It isn't obvious that blocking for a safepoint whilst in this method will always be a safe thing to do. That would need to be examined in detail - potential interactions can be very subtle. David Let me know what you think, Carsten
Re: Fwd: Patch: Clean up fix for JDK-8047720
Carsten, I've filed the following new bug to track this potential issue: JDK-8072439 fix for 8047720 may need more work https://bugs.openjdk.java.net/browse/JDK-8072439 Dan On 2/3/15 12:00 PM, Carsten Varming wrote: Greetings all, I was recently introduced to the deadlock described in JDK-8047720 and fixed in JDK9. The fix seems a little messy to me, and it looks like it left some very short races in the code. So I decided to clean up the code. See attached diff. The change takes a step back and acquires PeriodicTask_lock in WatcherThread::stop (like before the fix in JDK9), but this time safepoints are allowed to proceed when acquiring PeriodicTask_lock, preventing the deadlock. Let me know what you think, Carsten
Fwd: Patch: Clean up fix for JDK-8047720
Greetings all, I was recently introduced to the deadlock described in JDK-8047720 and fixed in JDK9. The fix seems a little messy to me, and it looks like it left some very short races in the code. So I decided to clean up the code. See attached diff. The change takes a step back and acquires PeriodicTask_lock in WatcherThread::stop (like before the fix in JDK9), but this time safepoints are allowed to proceed when acquiring PeriodicTask_lock, preventing the deadlock. Let me know what you think, Carsten patch Description: Binary data
Re: RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans
On 2/3/15 5:16 AM, shanliang wrote: Hi, Hope this is last time :) http://cr.openjdk.java.net/~sjiang/JDK-8065213/02/ Looks good. Some lines are quite long that are better to reformat (no need to send a new webrev). Mandy
Re: RFR(XS): 8072401, 8072403, 8072405: Small fixes to newly added DCMD tests
Looks good! Thanks, /Staffan > On 3 feb 2015, at 16:27, Mikael Auno wrote: > > Hi, could I please have some reviews for these very small fixes. > > Webrev: > http://cr.openjdk.java.net/~miauno/8072401_8072403_8072405/webrev.00/ > > Issues: > > Some of the newly added DCMD tests fail due to lack of -XX:+UsePerfData > https://bugs.openjdk.java.net/browse/JDK-8072401 > > HeapDumpTest and HeapDumpAllTest fails to find jhat in non-JDK runs > https://bugs.openjdk.java.net/browse/JDK-8072403 > > DCMD tests needs at least compact3 profile > https://bugs.openjdk.java.net/browse/JDK-8072405 > > Thanks, > Mikael
Re: RFR(XS): 8072401, 8072403, 8072405: Small fixes to newly added DCMD tests
Thanks for the review. Could you help push this for me? The exported
changsets are attached.
Thanks,
Mikael
On 2015-02-03 16:44, Jaroslav Bachorik wrote:
> Looks good.
>
> -JB-
>
> On 3.2.2015 16:27, Mikael Auno wrote:
>> Hi, could I please have some reviews for these very small fixes.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~miauno/8072401_8072403_8072405/webrev.00/
>>
>> Issues:
>>
>> Some of the newly added DCMD tests fail due to lack of -XX:+UsePerfData
>> https://bugs.openjdk.java.net/browse/JDK-8072401
>>
>> HeapDumpTest and HeapDumpAllTest fails to find jhat in non-JDK runs
>> https://bugs.openjdk.java.net/browse/JDK-8072403
>>
>> DCMD tests needs at least compact3 profile
>> https://bugs.openjdk.java.net/browse/JDK-8072405
>>
>> Thanks,
>> Mikael
>>
>
# HG changeset patch
# User miauno
# Date 1422962800 -3600
# Node ID 01cc42eb107afca5408e30a0a09c251be7976b98
# Parent 190387dac81353a3bc1dddbb328a60f2cb85500a
8072401: [TESTBUG] Some of the newly added DCMD tests fail due to lack of -XX:+UsePerfData
Reviewed-by: jbachorik
diff --git a/test/serviceability/dcmd/framework/HelpTest.java b/test/serviceability/dcmd/framework/HelpTest.java
--- a/test/serviceability/dcmd/framework/HelpTest.java
+++ b/test/serviceability/dcmd/framework/HelpTest.java
@@ -35,7 +35,7 @@
* @library /testlibrary
* @build com.oracle.java.testlibrary.*
* @build com.oracle.java.testlibrary.dcmd.*
- * @run testng HelpTest
+ * @run testng/othervm -XX:+UsePerfData HelpTest
*/
public class HelpTest {
public void run(CommandExecutor executor) {
diff --git a/test/serviceability/dcmd/framework/InvalidCommandTest.java b/test/serviceability/dcmd/framework/InvalidCommandTest.java
--- a/test/serviceability/dcmd/framework/InvalidCommandTest.java
+++ b/test/serviceability/dcmd/framework/InvalidCommandTest.java
@@ -35,7 +35,7 @@
* @library /testlibrary
* @build com.oracle.java.testlibrary.*
* @build com.oracle.java.testlibrary.dcmd.*
- * @run testng InvalidCommandTest
+ * @run testng/othervm -XX:+UsePerfData InvalidCommandTest
*/
public class InvalidCommandTest {
diff --git a/test/serviceability/dcmd/framework/VMVersionTest.java b/test/serviceability/dcmd/framework/VMVersionTest.java
--- a/test/serviceability/dcmd/framework/VMVersionTest.java
+++ b/test/serviceability/dcmd/framework/VMVersionTest.java
@@ -36,7 +36,7 @@
* @library /testlibrary
* @build com.oracle.java.testlibrary.*
* @build com.oracle.java.testlibrary.dcmd.*
- * @run testng VMVersionTest
+ * @run testng/othervm -XX:+UsePerfData VMVersionTest
*/
public class VMVersionTest {
public void run(CommandExecutor executor) {
# HG changeset patch
# User miauno
# Date 1422964153 -3600
# Node ID bca4aed2d75f4025d3d0929235d88155e36a0731
# Parent 01cc42eb107afca5408e30a0a09c251be7976b98
8072403: [TESTBUG] HeapDumpTest and HeapDumpAllTest fails to find jhat in non-JDK runs
Reviewed-by: jbachorik
diff --git a/test/serviceability/dcmd/gc/HeapDumpTest.java b/test/serviceability/dcmd/gc/HeapDumpTest.java
--- a/test/serviceability/dcmd/gc/HeapDumpTest.java
+++ b/test/serviceability/dcmd/gc/HeapDumpTest.java
@@ -51,7 +51,7 @@
}
private void verifyHeapDump(String fileName) {
-String jhat = JDKToolFinder.getTestJDKTool("jhat");
+String jhat = JDKToolFinder.getJDKTool("jhat");
String[] cmd = { jhat, "-parseonly", "true", fileName };
ProcessBuilder pb = new ProcessBuilder(cmd);
# HG changeset patch
# User miauno
# Date 1422964173 -3600
# Node ID 926a6fbb8d1ab01ef64e93b817f4c60705d5e37f
# Parent bca4aed2d75f4025d3d0929235d88155e36a0731
8072405: [TESTBUG] DCMD tests needs at least compact3 profile
Reviewed-by: jbachorik
diff --git a/test/TEST.groups b/test/TEST.groups
--- a/test/TEST.groups
+++ b/test/TEST.groups
@@ -145,7 +145,8 @@
gc/survivorAlignment \
runtime/InternalApi/ThreadCpuTimesDeadlock.java \
serviceability/threads/TestFalseDeadLock.java \
- compiler/codecache/jmx
+ compiler/codecache/jmx \
+ serviceability/dcmd
# Compact 2 adds full VM tests
compact2 = \
Re: RFR(XS): 8072401, 8072403, 8072405: Small fixes to newly added DCMD tests
Looks good. -JB- On 3.2.2015 16:27, Mikael Auno wrote: Hi, could I please have some reviews for these very small fixes. Webrev: http://cr.openjdk.java.net/~miauno/8072401_8072403_8072405/webrev.00/ Issues: Some of the newly added DCMD tests fail due to lack of -XX:+UsePerfData https://bugs.openjdk.java.net/browse/JDK-8072401 HeapDumpTest and HeapDumpAllTest fails to find jhat in non-JDK runs https://bugs.openjdk.java.net/browse/JDK-8072403 DCMD tests needs at least compact3 profile https://bugs.openjdk.java.net/browse/JDK-8072405 Thanks, Mikael
RFR(XS): 8072401, 8072403, 8072405: Small fixes to newly added DCMD tests
Hi, could I please have some reviews for these very small fixes. Webrev: http://cr.openjdk.java.net/~miauno/8072401_8072403_8072405/webrev.00/ Issues: Some of the newly added DCMD tests fail due to lack of -XX:+UsePerfData https://bugs.openjdk.java.net/browse/JDK-8072401 HeapDumpTest and HeapDumpAllTest fails to find jhat in non-JDK runs https://bugs.openjdk.java.net/browse/JDK-8072403 DCMD tests needs at least compact3 profile https://bugs.openjdk.java.net/browse/JDK-8072405 Thanks, Mikael
Re: jmx-dev RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans
Looks good to me Shanliang.
-- daniel
On 03/02/15 14:16, shanliang wrote:
Hi,
Hope this is last time :)
http://cr.openjdk.java.net/~sjiang/JDK-8065213/02/
1)
Mandy Chung wrote:
You may want to consider using limited doPrivileged (that can be done
in the second phase).
Done as in ManagementFactory:
878 // get all providers
879 List providers
880 =
AccessController.doPrivileged((PrivilegedAction>)
() -> {
881 List all = new
ArrayList<>();
882 for (PlatformMBeanProvider provider :
ServiceLoader.loadInstalled(PlatformMBeanProvider.class)) {
883 all.add(provider);
884 }
885 all.add(new DefaultPlatformMBeanProvider());
886 return all;
887 },
888 null,
889 new FilePermission("<>", "read"),
890 new
RuntimePermission("sun.management.spi.PlatformMBeanProvider"));
2)
The modification to Flag is removed, we get another solution to know
whether commercial feature is enabled.
3) some mis minors modifications.
Thanks,
Shanliang
Re: RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans
Hi,
Hope this is last time :)
http://cr.openjdk.java.net/~sjiang/JDK-8065213/02/
1)
Mandy Chung wrote:
You may want to consider using limited doPrivileged (that can be done
in the second phase).
Done as in ManagementFactory:
878 // get all providers
879 List providers
880 =
AccessController.doPrivileged((PrivilegedAction>)
() -> {
881 List all = new
ArrayList<>();
882 for (PlatformMBeanProvider provider :
ServiceLoader.loadInstalled(PlatformMBeanProvider.class)) {
883 all.add(provider);
884 }
885 all.add(new DefaultPlatformMBeanProvider());
886 return all;
887 },
888 null,
889 new FilePermission("<>", "read"),
890 new
RuntimePermission("sun.management.spi.PlatformMBeanProvider"));
2)
The modification to Flag is removed, we get another solution to know
whether commercial feature is enabled.
3) some mis minors modifications.
Thanks,
Shanliang
RFR(S, testonly): JDK-8072395: LingeredAppTest.java and JMapHeapConfigTest.java fail due to LingeredApp ERROR: java.io.IOException: Lock is too old. Aborting
Everyone, Please review the fix: http://cr.openjdk.java.net/~dsamersoff/JDK-8072395/webrev.01/ 1. I was wrong in my assumption that the test can't run more than an hour of physical machine time when wrote the test. This check is removed. 2. Added sanity check for common environment pitfalls to have better diagnostic in case of a fail 3. The test kept disabled on Mac OS X until the problem with attach permissions for non-root users is solved. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Jaroslav,
Looks good for me!
> I've changed the PortAllocator to allocate an array of unique random
> ports instead of letting ServerSocket to take care of it.
>
> I ran the test 200x in a tight loop without a failure.
>
> I hope this will resolve this test's intermittent failures due to port
> conflicts once and for all.
>
> Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.03
>
-Dmitry
On 2015-01-09 15:17, Jaroslav Bachorik wrote:
> Thank you all for the valuable input!
>
> On 9.1.2015 03:50, Stuart Marks wrote:
>> Hi Jaroslav,
>>
>> I'm distant enough from this code that I don't think I'm in a position
>> to say "no you can't check this in," and I'm mindful of the fact that
>> this bug is a high priority and you want to get a fix in. But having
>> said that I think there's a surprising amount of complexity here for
>> what it does.
>>
>> Implementing a retry-on-BindException policy seems to be fairly
>> sensible, since I assume it would be fairly invasive to modify the code
>> in the JVMs being forked to use an ephemeral port and send that
>> information back to the test.
>>
>> My conjecture is however that the open/close/reopen logic is the primary
>> cause of the BindExceptions that occur. If you're going to retry on
>> BindException, you might as well choose a random port number instead of
>> doing the open/close to get a port number from the system.
>>
>> The range that Dmitry suggests is reasonable, though I note that the
>> actual ephemeral port range used by the kernel will differ from OS to OS
>> and even from system to system. I don't know if that's really
>> significant though. If you end up choosing a port outside the ephemeral
>> range for some system, does it really matter?
>>
>> If you do decide to have PortAllocator open and close a ServerSocket (in
>> order to find a previously unused port) I'd suggest removing the logic
>> that leaves the socket open until the first call to get(). That logic
>> reduces the possibility that some other process will open the socket
>> after the close but before the reopen. In my experience that's not
>> what's causing the BindExceptions. It could still happen, though, but
>> you're protected by the retry logic anyway. Leaving the socket open
>> longer actually hurts, I think, because it increases the chance that the
>> kernel won't have cleaned up the port by the time the test wants to
>> reopen it.
>>
>> If you change PortAllocator to close the socket immediately, you can get
>> rid of the need to call release() in a finally-block of the caller. You
>> could also change the type of the functional interface to be
>>
>> int[] -> void
>>
>> since the PortAllocator doesn't hold onto any resources that need to be
>> cleaned up. It just calls the execute() method and passes an array of n
>> port numbers.
>>
>> It's probably necessary to have the socket close() call in a retry loop.
>> The socket is always closed immediately from the user process point of
>> view, so isClosed() will always return true immediately after the
>> close() call returns. You can verify this easily by looking in the
>> ServerSocket.java source code. I believe the state that prevents the
>> port from being reused immediately is private to the kernel and cannot
>> be observed from a user process, at least not without attempting to
>> reopen the socket.
>>
>> Side note: one of the jcmd() overloads says that parameter 'c' (a
>> Consumer) may be null. It doesn't look like this is handled. If you
>> really want to support this, I'd assign () -> { } to it if it's null so
>> that it can be called unconditionally. (Or just disallow null.)
>
> I've changed the PortAllocator to allocate an array of unique random
> ports instead of letting ServerSocket to take care of it.
>
> I ran the test 200x in a tight loop without a failure.
>
> I hope this will resolve this test's intermittent failures due to port
> conflicts once and for all.
>
> Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.03
>
> Thanks,
>
> -JB-
>
>
>>
>> s'marks
>>
>>
>> On 1/6/15 2:00 PM, Dmitry Samersoff wrote:
>>> Jaroslav,
>>>
>>> It might be better to just choose a random digit between 49152–65535
>>> and attempt to use it.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2014-12-18 17:29, Jaroslav Bachorik wrote:
On 12/11/2014 03:43 PM, Dmitry Samersoff wrote:
> Jaroslav,
>
> You can set SO_LINGER to zero, in this case socket will be closed
> immediately without waiting in TIME_WAIT
>
> But there are no reliable way to predict whether you can take this
> port
> or not after you close it.
>
> So the only valid solution is to try to connect to a random port
> and if
> this attempt fails try another random port. Everything else will cause
> more or less frequent intermittent failures.
Thanks for all the suggestions!
http://cr.openjdk.java.net/~jbachorik/8066708/webrev.02
I've enhanced the original patch with the retry lo
Re: RFR: JDK-8068589: GCCause should distinguish jcmd GC.run from System.gc()
Hi, Not an official review but it’s a simple patch that looks good. Kind regards, Kirk On Feb 2, 2015, at 4:51 PM, Yasumasa Suenaga wrote: > Hi, > > I need more reviewer. > Could you review it? > > http://cr.openjdk.java.net/~ysuenaga/JDK-8068589/webrev.02/ > > Thanks, > > Yasumasa > > 2015/01/28 17:24 "Staffan Larsen" : > Looks good! > > Thanks, > /Staffan > > > On 28 jan 2015, at 05:48, Yasumasa Suenaga wrote: > > > > Hi Staffan, Kirk, > > > > I agree to set "Diagnostic Command" to GCCause. > > So I applied it to new patch. > > > > http://cr.openjdk.java.net/~ysuenaga/JDK-8068589/webrev.02/ > > > > Could you review it again? > > > > > > Thanks, > > > > Yasumasa > > > > > > On 2015/01/28 5:06, Kirk Pepperdine wrote: > >> Hi Staffan, > >> > > Anyway, it’s a record in a GC log so I don’t see the value of GC.run. > Certainly “DiagCmd" or even "Diagnostic Command” seems sufficient given > the context. > >>> > >>> Let’s go with “Diagnostic Command”, then. > >> > >> Thank you! > >> > >> Regards, > >> Kirk > >> > signature.asc Description: Message signed with OpenPGP using GPGMail
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
On 2.2.2015 23:09, Stuart Marks wrote:
On 2/2/15 2:33 AM, Jaroslav Bachorik wrote:
I've applied your comments and the code is a tad simpler now. I also
ironed out
a few more corner cases. I ran the test 500x in a tight loop and no
failure, yay!
Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.04
Hi Jaroslav,
Looks quite a bit more straightforward now. I'm pretty much OK with this
if you're OK with it; I think I've reviewed it enough times already. :-)
I have a couple comments on test_09 that you might want to address
before you push or sometime in the future.
Line 714 typo "hugging" => "hogging"
D'oh. Thanks.
But I'm not convinced this retry logic in the while-loop from lines
716-723 is necessary. If you've already opened a server socket on a
port, I've never seen a case where opening the same port *again* will
succeed, so why bother?
I saw the test failing without this retry logic. The purpose is to make
sure that the port is really taken before the test continues.
I'd suggest simply opening ServerSocket(0) and then getting the port via
getLocalPort(). I've never seen this fail. This case should work since
you actually want to open up some random port, instead of generating a
random port number for somebody else (the subprocess) to open. You can
then allocate one fewer random port. You might want to have a little
loop to check that the random port number isn't a duplicate of the
actual port that you just opened. I think this lets the code boil down a
bit further:
ServerSocket ss = new ServerSocket(0);
try {
int localPort = ss.getLocalPort();
int[] ports;
do {
ports = PortAllocator.allocatePorts(1);
} while (localPort != ports[0]);
AtomicBoolean checks = new AtomicBoolean(false);
...
} finally {
ss.close();
s.stop();
}
I'll give it a try and see how this behaves.
Thanks,
-JB-
s'marks
