RFR (M): 8145435: [JVMCI] some tests on Windows fail with: assert(!thread->is_Java_thread()) failed: must not be java thread

David Holmes david.holmes at oracle.com
Wed Dec 16 00:54:49 UTC 2015

Hi Christian,


tl;dr: the new assert is wrong, don't add it.

os::sleep has two paths:

1. interruptible sleeps

This is the implementation of java.lang.Thread.sleep and should only be 
called by JavaThreads using that API (but see below)

2. non-interruptible sleep

Used for 'incidental' sleeps within the VM, primarily by 
non-JavaThreads. If a JavaThread uses this it will delay safepoints, so 
the sleep should be very small.

As you have discovered the Windows implementation of os::sleep actually 
asserts that JavaThreads don't take the second path. That's not correct. 
There are two existing cases where this will trigger today.

The first is a historical usage of ConvertYieldToSleep:

JVM_ENTRY(void, JVM_Yield(JNIEnv *env, jclass threadClass))
   if (os::dont_yield()) return;

   // When ConvertYieldToSleep is off (default), this matches the 
classic VM use of yield.
   // Critical for similar threading behaviour
   if (ConvertYieldToSleep) {
     os::sleep(thread, MinSleepInterval, false);
   } else {

Then in JVM_Sleep itself:

  if (millis == 0) {
     if (ConvertSleepToYield) {
     } else {
       ThreadState old_state = thread->osthread()->get_state();
       os::sleep(thread, MinSleepInterval, false);

This would also blow up on Windows - if ConvertSleepToYield is turned 
off (it is on by default on x86 - in fact all platforms except sparc - 
which really should be a Solaris setting as it will now affect the 
Linux/sparc port (which would then crash with your proposed additional 
assert in os_posix.cpp!).

So adding the assert to os::sleep on Posix is not correct, and arguably 
the assertion should be removed from the windows code.



If you grep you will find about a 50-50 split between interruptible and 
interruptable through the codebase, so unless you want to file a cleanup 
bug to make it consistent I would just drop this change.



It is not at all clear to me will be in a position to print the 
exception information correctly at this stage of the termination logic. 
I would argue that any errors during JVMCIRuntime::shutdown should be 
handled internally and logged using the logging mechanism, not via 
exceptions and exception printing.




+   // Give other aborting threads to also print their stack traces.

should be:

+   // Give other aborting threads a chance to also print their stack 

though I am dubious about the need for this and the use of the os::sleep 
that seemed to trigger this whole issue. Why aren't you using a normal 
vmError-based termination path for such fatal errors?


On 16/12/2015 6:54 AM, Christian Thalinger wrote:
> https://bugs.openjdk.java.net/browse/JDK-8145435
> http://cr.openjdk.java.net/~twisti/8145435/webrev/
> The actual fix for the problem is passing true to os::sleep:
> +  const bool interruptible = true;
> +  os::sleep(THREAD, 200, interruptible);
> since compiler threads are Java threads but I fixed a couple more things...
> First, I’ve added the same assert to os_posix.cpp:
> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/os/posix/vm/os_posix.cpp.udiff.html
> because it’s odd to only hit this assert on Windows:
> Additionally I’ve changed the way we handle an exception in before_exit:
> http://cr.openjdk.java.net/~twisti/8145435/webrev/src/share/vm/runtime/java.cpp.udiff.html
> and moved abort_on_pending_exception into JVMCICompiler and made it private.  It’s used for one thing only now.
> Finally I replaced JVMCIRuntime::call_printStackTrace with calls to java_lang_Throwable::print_stack_trace.

More information about the hotspot-dev mailing list