From 6b8486d59143177c785c7a39a49e63d68bc1fafa Mon Sep 17 00:00:00 2001 From: Salikh Zakirov Date: Wed, 28 Feb 2007 17:22:40 +0300 Subject: [PATCH] (WIP) explicit thread block lifecycle * vm_detach(): reset HyThread pointer * thread_start_proc(): zero out HyThread after thread terminated * Thread.java: use synchronized(lock) before calling into VMThreadmanager Note that this patch is a work-in-progress (WIP) and is not yet freeing the thread blocks as it should. Instead, it fills old thread blocks with zero for easy detection of the dangling pointer bugs. Deallocating thread blocks on thread termination implies that some interfaces become inherently unsafe. The only current user of the unsafe interfaces is VM shutdown stage, which stops the daemon java threads before proceeding to the deinitializing VM resources. This patch rewrites this phase in Java, but both this solution and former solution are deficient, and need to be rewritten using different mechanism, see HARMONY-3272 for more details. Use static inner class for a thread lock for easier identification of thread lock in the debugger. --- vm/thread/src/thread_java_basic.c | 8 -- vm/thread/src/thread_native_basic.c | 22 +++++ vm/vmcore/src/init/vm_shutdown.cpp | 21 +++++ .../kernel_classes/javasrc/java/lang/Thread.java | 85 ++++++++++++-------- .../javasrc/java/lang/ThreadGroup.java | 34 ++++++++ vm/vmcore/src/thread/thread_generic.cpp | 3 + 6 files changed, 130 insertions(+), 43 deletions(-) diff --git a/vm/thread/src/thread_java_basic.c b/vm/thread/src/thread_java_basic.c index e5e7bdd..589322b 100644 --- a/vm/thread/src/thread_java_basic.c +++ b/vm/thread/src/thread_java_basic.c @@ -239,16 +239,8 @@ IDATA jthread_attach(JNIEnv * jni_env, jthread java_thread, jboolean daemon) { */ jlong jthread_thread_init(jvmti_thread_t *ret_thread, JNIEnv* env, jthread java_thread, jobject weak_ref, jlong old_thread) { hythread_t tm_native_thread = NULL; - jvmti_thread_t tmj_thread; IDATA status; - if (old_thread) { - tm_native_thread = (hythread_t)((IDATA)old_thread); - tmj_thread = (jvmti_thread_t)hythread_get_private_data(tm_native_thread); - //delete used weak reference - if (tmj_thread->thread_ref) (*env)->DeleteGlobalRef(env, tmj_thread->thread_ref); - } - status = hythread_struct_init(&tm_native_thread); if (status != TM_ERROR_NONE) { return 0; diff --git a/vm/thread/src/thread_native_basic.c b/vm/thread/src/thread_native_basic.c index 3d0e930..9c836dd 100644 --- a/vm/thread/src/thread_native_basic.c +++ b/vm/thread/src/thread_native_basic.c @@ -649,6 +649,25 @@ static hythread_t allocate_thread() { return ptr; } +static void free_thread(hythread_t thread) { + int r; + assert(thread->group == NULL && "freeing non-detached thread"); + // latch and sem are malloced + r = hylatch_destroy(thread->join_event); + assert(r == TM_ERROR_NONE); + r = hylatch_destroy(thread->safe_region_event); + assert(r == TM_ERROR_NONE); + r = hysem_destroy(thread->resume_event); + assert(r == TM_ERROR_NONE); + + // cond and mutex are inline + r = hymutex_destroy(&thread->mutex); + assert(r == TM_ERROR_NONE); + r = hycond_destroy(&thread->condition); + assert(r == TM_ERROR_NONE); + free(thread); +} + static void reset_thread(hythread_t thread) { int r; IDATA status; @@ -714,9 +733,12 @@ static int VMAPICALL thread_start_proc(void *arg) { thread->exit_value = 0; hythread_detach(thread); + // Send join event to those threads who called join on this thread. hylatch_count_down(thread->join_event); + free_thread(thread); + status = hythread_global_unlock(NULL); assert(status == TM_ERROR_NONE); diff --git a/vm/vmcore/src/init/vm_shutdown.cpp b/vm/vmcore/src/init/vm_shutdown.cpp index 9d52080..d16c9d1 100644 --- a/vm/vmcore/src/init/vm_shutdown.cpp +++ b/vm/vmcore/src/init/vm_shutdown.cpp @@ -146,6 +146,27 @@ static void vm_shutdown_stop_java_threads(Global_Env * vm_env) { TRACE2("shutdown", "shutting down threads complete"); } +/* +static void vm_shutdown_stop_java_threads(JNIEnv *jni_env) +{ + // + // !!! Working with native threads directly is inherently incorrect, + // !!! as they can terminate and deallocate thread block while + // !!! we are using it. + // + // Call ThreadGroup.shutdown() instead + // + jclass thread_class = jni_env->FindClass("java/lang/ThreadGroup"); + assert(thread_class); + + jmethodID shutdown_method = + jni_env->GetStaticMethodID(thread_class, "shutdown", "()V"); + assert(shutdown_method); + + jni_env->CallStaticVoidMethod(thread_class, shutdown_method); +} +*/ + /** * Waits until all non-daemon threads finish their execution, * initiates VM shutdown sequence and stops running threads if any. diff --git a/vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java b/vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java index 4132902..61e652d 100644 --- a/vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java +++ b/vm/vmcore/src/kernel_classes/javasrc/java/lang/Thread.java @@ -130,10 +130,11 @@ public class Thread implements Runnable { */ private static long threadOrdinalNum = 0; + static class Lock {} /** * Synchronization is done using internal lock. */ - Object lock = new Object(); + Object lock = new Lock(); /** * used to generate a default thread name @@ -550,6 +551,9 @@ public class Thread implements Runnable { public void interrupt() { synchronized (lock) { checkAccess(); + + if (!isAlive()) return; + int status = VMThreadManager.interrupt(this); if (status != VMThreadManager.TM_ERROR_NONE) { throw new InternalError( @@ -562,9 +566,7 @@ public class Thread implements Runnable { * @com.intel.drl.spec_ref */ public final boolean isAlive() { - synchronized (lock) { - return this.isAlive; - } + return this.isAlive; } @@ -579,7 +581,10 @@ public class Thread implements Runnable { * @com.intel.drl.spec_ref */ public boolean isInterrupted() { - return VMThreadManager.isInterrupted(this); + synchronized (lock) { + if (!isAlive()) return false; + return VMThreadManager.isInterrupted(this); + } } /** @@ -629,7 +634,7 @@ public class Thread implements Runnable { long end = System.nanoTime() + 1000000*millis + (long)nanos; long rest; while (isAlive()) { - lock.wait(millis, nanos); + lock.wait(millis, nanos); rest = end - System.nanoTime(); if (rest <= 0) return; nanos = (int)(rest % 1000000); @@ -645,12 +650,17 @@ public class Thread implements Runnable { * @com.intel.drl.spec_ref */ public final void resume() { + synchronized (lock) { checkAccess(); + + if (!isAlive()) return; + int status = VMThreadManager.resume(this); if (status != VMThreadManager.TM_ERROR_NONE) { throw new InternalError( "Thread Manager internal error " + status); } + } } /** @@ -721,9 +731,12 @@ public class Thread implements Runnable { ThreadGroup threadGroup = group; this.priority = (priority > threadGroup.maxPriority) ? threadGroup.maxPriority : priority; - int status = VMThreadManager.setPriority(this, this.priority); - if (status != VMThreadManager.TM_ERROR_NONE) { - //throw new InternalError("Thread Manager internal error " + status); + + synchronized (lock) { + int status = VMThreadManager.setPriority(this, this.priority); + if (status != VMThreadManager.TM_ERROR_NONE) { + //throw new InternalError("Thread Manager internal error " + status); + } } } @@ -793,27 +806,30 @@ public class Thread implements Runnable { */ public Thread.State getState() { - boolean dead = false; synchronized(lock) { - if(started && !isAlive() ) dead = true; - } - if (dead) return State.TERMINATED; - - int state = (VMThreadManager.getState(this)); - - if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_TERMINATED)) { - return State.TERMINATED; - } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT)) { - return State.TIMED_WAITING; - } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_WAITING) - || 0 != (state & VMThreadManager.JVMTI_THREAD_STATE_PARKED)) { - return State.WAITING; - } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER)) { - return State.BLOCKED; - } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_ALIVE)) { - return State.RUNNABLE; - } else { - return State.NEW; + if (!isAlive()) { + if (!started) + return State.NEW; + else + return State.TERMINATED; + } + + int state = (VMThreadManager.getState(this)); + + if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_TERMINATED)) { + return State.TERMINATED; + } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_WAITING_WITH_TIMEOUT)) { + return State.TIMED_WAITING; + } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_WAITING) + || 0 != (state & VMThreadManager.JVMTI_THREAD_STATE_PARKED)) { + return State.WAITING; + } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_BLOCKED_ON_MONITOR_ENTER)) { + return State.BLOCKED; + } else if (0 != (state & VMThreadManager.JVMTI_THREAD_STATE_ALIVE)) { + return State.RUNNABLE; + } else { + return State.NEW; + } } } @@ -821,11 +837,7 @@ public class Thread implements Runnable { * @com.intel.drl.spec_ref */ public final void stop() { - synchronized (lock) { - if (isAlive()) { - stop(new ThreadDeath()); - } - } + stop(new ThreadDeath()); } /** @@ -861,12 +873,15 @@ public class Thread implements Runnable { * @com.intel.drl.spec_ref */ public final void suspend() { - checkAccess(); + checkAccess(); + synchronized (lock) { + if (!isAlive()) return; int status = VMThreadManager.suspend(this); if (status != VMThreadManager.TM_ERROR_NONE) { throw new InternalError( "Thread Manager internal error " + status); } + } } /** diff --git a/vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java b/vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java index ef14a78..1671891 100644 --- a/vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java +++ b/vm/vmcore/src/kernel_classes/javasrc/java/lang/ThreadGroup.java @@ -641,4 +641,38 @@ public class ThreadGroup implements Thread.UncaughtExceptionHandler{ } } } + + /** + * Stops unfinished threads + */ + public static void shutdown() { + ThreadGroup group = Thread.currentThread().group; + // we use group.parent rather than getParent() to skip security check + while (group.parent != null) + group = group.parent; + shutdown(group); + } + + private static void shutdown(ThreadGroup group) { + Iterator it = group.threads.keySet().iterator(); + Thread self = Thread.currentThread(); + while (it.hasNext()) { + Thread t = it.next(); + if (t == self) + continue; + + t.stop(); + + // make sure thread is really stopped + try { t.join(); } catch (InterruptedException e) {} + if (t.isAlive()) + System.err.println("WARNING: thread " + t + " could not be stopped"); + } + + // recursively shutdown other thread groups + Iterator ig = group.groups.iterator(); + while (ig.hasNext()) { + shutdown(ig.next()); + } + } } diff --git a/vm/vmcore/src/thread/thread_generic.cpp b/vm/vmcore/src/thread/thread_generic.cpp index 72f9bd8..da95ed4 100644 --- a/vm/vmcore/src/thread/thread_generic.cpp +++ b/vm/vmcore/src/thread/thread_generic.cpp @@ -279,6 +279,9 @@ jint vm_detach(jthread java_thread) { status = run_java_detach(java_thread); if (status != JNI_OK) return status; + // reset hythread_t pointer in java object + vm_jthread_set_tm_data(java_thread, NULL); + // Send Thread End event jvmti_send_thread_start_end_event(0); -- 1.4.1.g4455