Thanks for the quick review Serguei!
One more review would be nice... David H. or Thomas Stüfe?
On 6/26/18 12:12 PM, serguei.spit...@oracle.com wrote:
Hi Dan,
The fix looks reasonable to me.
Thanks. I like simple one liners... :-)
Nice catch by David!
Definitely.
Dan
Thanks,
Serguei
On 6/26/18 06:06, Daniel D. Daugherty wrote:
Greetings,
For this one liner review, I'd love to hear from David H., Thomas Stüfe,
and Serguei Spitsyn.
David Holmes spotted a locking problem with my fix for the following
bug:
JDK-8205195 NestedThreadsListHandleInErrorHandlingTest fails because
hs_err doesn't contain _nested_thread_list_max
https://bugs.openjdk.java.net/browse/JDK-8205195
Webrev URL:
http://cr.openjdk.java.net/~dcubed/8205195-webrev/0-for-jdk-jdk/
The fix for the issue is a one liner that only grabs the Threads_lock
when the caller does not already own it:
$ hg diff
diff -r 5698cf4e50f1 src/hotspot/share/utilities/vmError.cpp
--- a/src/hotspot/share/utilities/vmError.cpp Fri Jun 22 12:15:16
2018 -0400
+++ b/src/hotspot/share/utilities/vmError.cpp Mon Jun 25 21:20:52
2018 -0400
@@ -1703,7 +1703,7 @@
// from racing with Threads::add() or Threads::remove() as we
// generate the hs_err_pid file. This makes our ErrorHandling tests
// more stable.
- MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
+ MutexLockerEx ml(Threads_lock->owned_by_self() ? NULL :
Threads_lock, Mutex::_no_safepoint_check_flag);
switch (how) {
case 1: vmassert(str == NULL, "expected null"); break;
Figuring out a way to detect the failure mode in order to test the
fix was much more involved than the fix itself. Gory details are
in the bug:
JDK-8205648 fix for 8205195 breaks secondary error handling
https://bugs.openjdk.java.net/browse/JDK-8205648
No webrev since the one liner diff is shown above. This fix was tested
with the test/hotspot/jtreg/runtime/ErrorHandling tests on Linux-X64
and with a special version of ErrorHandling/SecondaryErrorTest.java
that is documented in JDK-8205648.
Thanks, in advance, for any comments, questions or suggestions.
Dan
