8214097: Rework thread initialization and teardown logic

Reviewed-by: rehn, mgronlun, dcubed, kbarrett

src/hotspot/os/linux/os_linux.cpp

@@ -705,6 +705,8 @@ static void *thread_native_entry(Thread *thread) {
     }
   }
 
+  assert(osthread->pthread_id() != 0, "pthread_id was not set as expected");
+
   // call one more level start routine
   thread->call_run();
 

src/hotspot/share/gc/parallel/gcTaskThread.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -114,7 +114,6 @@ void GCTaskThread::print_task_time_stamps() {
 // for tasks to be enqueued for execution.
 
 void GCTaskThread::run() {
-  this->initialize_named_thread();
   // Bind yourself to your processor.
   if (processor_id() != GCTaskManager::sentinel_worker()) {
     log_trace(gc, task, thread)("GCTaskThread::run: binding to processor %u", processor_id());

src/hotspot/share/gc/shared/concurrentGCThread.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -49,7 +49,6 @@ void ConcurrentGCThread::create_and_start(ThreadPriority prio) {
 }
 
 void ConcurrentGCThread::initialize_in_thread() {
-  this->initialize_named_thread();
   this->set_active_handles(JNIHandleBlock::allocate_block());
   // From this time Thread::current() should be working.
   assert(this == Thread::current(), "just checking");

src/hotspot/share/gc/shared/workgroup.cpp

@@ -297,7 +297,6 @@ void AbstractGangWorker::run() {
 }
 
 void AbstractGangWorker::initialize() {
-  this->initialize_named_thread();
   assert(_gang != NULL, "No gang to run in");
   os::set_priority(this, NearMaxPriority);
   log_develop_trace(gc, workgang)("Running gang worker for gang %s id %u", gang()->name(), id());

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp

@@ -335,7 +335,8 @@ class JfrThreadSampler : public NonJavaThread {
   void set_native_interval(size_t interval) { _interval_native = interval; };
   size_t get_java_interval() { return _interval_java; };
   size_t get_native_interval() { return _interval_native; };
-
+ protected:
+  virtual void post_run();
  public:
   void run();
   static Monitor* transition_block() { return JfrThreadSampler_lock; }
@@ -484,6 +485,10 @@ void JfrThreadSampler::run() {
       last_native_ms = get_monotonic_ms();
     }
   }
+}
+
+void JfrThreadSampler::post_run() {
+  this->NonJavaThread::post_run();
   delete this;
 }
 

src/hotspot/share/runtime/thread.cpp

@@ -213,8 +213,12 @@ void JavaThread::smr_delete() {
 // Base class for all threads: VMThread, WatcherThread, ConcurrentMarkSweepThread,
 // JavaThread
 
+DEBUG_ONLY(Thread* Thread::_starting_thread = NULL;)
 
 Thread::Thread() {
+
+  DEBUG_ONLY(_run_state = PRE_CALL_RUN;)
+
   // stack and get_thread
   set_stack_base(NULL);
   set_stack_size(0);
@@ -314,11 +318,11 @@ Thread::Thread() {
   if (barrier_set != NULL) {
     barrier_set->on_thread_create(this);
   } else {
-#ifdef ASSERT
-    static bool initial_thread_created = false;
-    assert(!initial_thread_created, "creating thread before barrier set");
-    initial_thread_created = true;
-#endif // ASSERT
+    // Only the main thread should be created before the barrier set
+    // and that happens just before Thread::current is set. No other thread
+    // can attach as the VM is not created yet, so they can't execute this code.
+    // If the main thread creates other threads before the barrier set that is an error.
+    assert(Thread::current_or_null() == NULL, "creating thread before barrier set");
   }
 }
 
@@ -368,9 +372,16 @@ void Thread::register_thread_stack_with_NMT() {
 #endif // INCLUDE_NMT
 
 void Thread::call_run() {
+  DEBUG_ONLY(_run_state = CALL_RUN;)
+
   // At this point, Thread object should be fully initialized and
   // Thread::current() should be set.
 
+  assert(Thread::current_or_null() != NULL, "current thread is unset");
+  assert(Thread::current_or_null() == this, "current thread is wrong");
+
+  // Perform common initialization actions
+
   register_thread_stack_with_NMT();
 
   JFR_ONLY(Jfr::on_thread_start(this);)
@@ -380,25 +391,42 @@ void Thread::call_run() {
     os::current_thread_id(), p2i(stack_base() - stack_size()),
     p2i(stack_base()), stack_size()/1024);
 
+  // Perform <ChildClass> initialization actions
+  DEBUG_ONLY(_run_state = PRE_RUN;)
+  this->pre_run();
+
   // Invoke <ChildClass>::run()
+  DEBUG_ONLY(_run_state = RUN;)
   this->run();
   // Returned from <ChildClass>::run(). Thread finished.
 
-  // Note: at this point the thread object may already have deleted itself.
-  // So from here on do not dereference *this*.
+  // Perform common tear-down actions
 
-  // If a thread has not deleted itself ("delete this") as part of its
-  // termination sequence, we have to ensure thread-local-storage is
-  // cleared before we actually terminate. No threads should ever be
-  // deleted asynchronously with respect to their termination.
-  if (Thread::current_or_null_safe() != NULL) {
-    assert(Thread::current_or_null_safe() == this, "current thread is wrong");
-    Thread::clear_thread_current();
-  }
+  assert(Thread::current_or_null() != NULL, "current thread is unset");
+  assert(Thread::current_or_null() == this, "current thread is wrong");
 
+  // Perform <ChildClass> tear-down actions
+  DEBUG_ONLY(_run_state = POST_RUN;)
+  this->post_run();
+
+  // Note: at this point the thread object may already have deleted itself,
+  // so from here on do not dereference *this*. Not all thread types currently
+  // delete themselves when they terminate. But no thread should ever be deleted
+  // asynchronously with respect to its termination - that is what _run_state can
+  // be used to check.
+
+  assert(Thread::current_or_null() == NULL, "current thread still present");
 }
 
 Thread::~Thread() {
+
+  // Attached threads will remain in PRE_CALL_RUN, as will threads that don't actually
+  // get started due to errors etc. Any active thread should at least reach post_run
+  // before it is deleted (usually in post_run()).
+  assert(_run_state == PRE_CALL_RUN ||
+         _run_state == POST_RUN, "Active Thread deleted before post_run(): "
+         "_run_state=%d", (int)_run_state);
+
   // Notify the barrier set that a thread is being destroyed. Note that a barrier
   // set might not be available if we encountered errors during bootstrapping.
   BarrierSet* const barrier_set = BarrierSet::barrier_set();
@@ -445,9 +473,10 @@ Thread::~Thread() {
   // osthread() can be NULL, if creation of thread failed.
   if (osthread() != NULL) os::free_thread(osthread());
 
-  // clear Thread::current if thread is deleting itself.
+  // Clear Thread::current if thread is deleting itself and it has not
+  // already been done. This must be done before the memory is deallocated.
   // Needed to ensure JNI correctly detects non-attached threads.
-  if (this == Thread::current()) {
+  if (this == Thread::current_or_null()) {
     Thread::clear_thread_current();
   }
 
@@ -1051,7 +1080,10 @@ bool Thread::is_lock_owned(address adr) const {
 }
 
 bool Thread::set_as_starting_thread() {
+  assert(_starting_thread == NULL, "already initialized: "
+         "_starting_thread=" INTPTR_FORMAT, p2i(_starting_thread));
   // NOTE: this must be called inside the main thread.
+  DEBUG_ONLY(_starting_thread = this;)
   return os::create_main_thread((JavaThread*)this);
 }
 
@@ -1254,27 +1286,48 @@ void NonJavaThread::Iterator::step() {
 }
 
 NonJavaThread::NonJavaThread() : Thread(), _next(NULL) {
-  // Add this thread to _the_list.
+  assert(BarrierSet::barrier_set() != NULL, "NonJavaThread created too soon!");
+}
+
+NonJavaThread::~NonJavaThread() { }
+
+void NonJavaThread::add_to_the_list() {
   MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
   _next = _the_list._head;
   OrderAccess::release_store(&_the_list._head, this);
 }
 
-NonJavaThread::~NonJavaThread() {
-  JFR_ONLY(Jfr::on_thread_exit(this);)
-  // Remove this thread from _the_list.
+void NonJavaThread::remove_from_the_list() {
   MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
   NonJavaThread* volatile* p = &_the_list._head;
   for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) {
     if (t == this) {
       *p = this->_next;
-      // Wait for any in-progress iterators.
+      // Wait for any in-progress iterators.  Concurrent synchronize is
+      // not allowed, so do it while holding the list lock.
       _the_list._protect.synchronize();
       break;
     }
   }
 }
 
+void NonJavaThread::pre_run() {
+  assert(BarrierSet::barrier_set() != NULL, "invariant");
+  add_to_the_list();
+
+  // This is slightly odd in that NamedThread is a subclass, but
+  // in fact name() is defined in Thread
+  assert(this->name() != NULL, "thread name was not set before it was started");
+  this->set_native_thread_name(this->name());
+}
+
+void NonJavaThread::post_run() {
+  JFR_ONLY(Jfr::on_thread_exit(this);)
+  remove_from_the_list();
+  // Ensure thread-local-storage is cleared before termination.
+  Thread::clear_thread_current();
+}
+
 // NamedThread --  non-JavaThread subclasses with multiple
 // uniquely named instances should derive from this.
 NamedThread::NamedThread() :
@@ -1301,10 +1354,6 @@ void NamedThread::set_name(const char* format, ...) {
   va_end(ap);
 }
 
-void NamedThread::initialize_named_thread() {
-  set_native_thread_name(name());
-}
-
 void NamedThread::print_on(outputStream* st) const {
   st->print("\"%s\" ", name());
   Thread::print_on(st);
@@ -1401,7 +1450,6 @@ int WatcherThread::sleep() const {
 void WatcherThread::run() {
   assert(this == watcher_thread(), "just checking");
 
-  this->set_native_thread_name(this->name());
   this->set_active_handles(JNIHandleBlock::allocate_block());
   while (true) {
     assert(watcher_thread() == Thread::current(), "thread consistency check");
@@ -1757,19 +1805,30 @@ JavaThread::~JavaThread() {
 }
 
 
-// The first routine called by a new Java thread
+// First JavaThread specific code executed by a new Java thread.
+void JavaThread::pre_run() {
+  // empty - see comments in run()
+}
+
+// The main routine called by a new Java thread. This isn't overridden
+// by subclasses, instead different subclasses define a different "entry_point"
+// which defines the actual logic for that kind of thread.
 void JavaThread::run() {
   // initialize thread-local alloc buffer related fields
   this->initialize_tlab();
 
-  // used to test validity of stack trace backs
+  // Used to test validity of stack trace backs.
+  // This can't be moved into pre_run() else we invalidate
+  // the requirement that thread_main_inner is lower on
+  // the stack. Consequently all the initialization logic
+  // stays here in run() rather than pre_run().
   this->record_base_of_stack_pointer();
 
   this->create_stack_guard_pages();
 
   this->cache_global_variables();
 
-  // Thread is now sufficient initialized to be handled by the safepoint code as being
+  // Thread is now sufficiently initialized to be handled by the safepoint code as being
   // in the VM. Change thread state from _thread_new to _thread_in_vm
   ThreadStateTransition::transition_and_fence(this, _thread_new, _thread_in_vm);
 
@@ -1788,13 +1847,10 @@ void JavaThread::run() {
   }
 
   // We call another function to do the rest so we are sure that the stack addresses used
-  // from there will be lower than the stack base just computed
+  // from there will be lower than the stack base just computed.
   thread_main_inner();
-
-  // Note, thread is no longer valid at this point!
 }
 
-
 void JavaThread::thread_main_inner() {
   assert(JavaThread::current() == this, "sanity check");
   assert(this->threadObj() != NULL, "just checking");
@@ -1814,10 +1870,16 @@ void JavaThread::thread_main_inner() {
 
   DTRACE_THREAD_PROBE(stop, this);
 
-  this->exit(false);
-  this->smr_delete();
+  // Cleanup is handled in post_run()
 }
 
+// Shared teardown for all JavaThreads
+void JavaThread::post_run() {
+  this->exit(false);
+  // Defer deletion to here to ensure 'this' is still referenceable in call_run
+  // for any shared tear-down.
+  this->smr_delete();
+}
 
 static void ensure_join(JavaThread* thread) {
   // We do not need to grab the Threads_lock, since we are operating on ourself.

src/hotspot/share/runtime/thread.hpp

@@ -110,6 +110,27 @@ class WorkerThread;
 // This means !t->is_Java_thread() iff t is a NonJavaThread, or t is
 // a partially constructed/destroyed Thread.
 
+// Thread execution sequence and actions:
+// All threads:
+//  - thread_native_entry  // per-OS native entry point
+//    - stack initialization
+//    - other OS-level initialization (signal masks etc)
+//    - handshake with creating thread (if not started suspended)
+//    - this->call_run()  // common shared entry point
+//      - shared common initialization
+//      - this->pre_run()  // virtual per-thread-type initialization
+//      - this->run()      // virtual per-thread-type "main" logic
+//      - shared common tear-down
+//      - this->post_run()  // virtual per-thread-type tear-down
+//      - // 'this' no longer referenceable
+//    - OS-level tear-down (minimal)
+//    - final logging
+//
+// For JavaThread:
+//   - this->run()  // virtual but not normally overridden
+//     - this->thread_main_inner()  // extra call level to ensure correct stack calculations
+//       - this->entry_point()  // set differently for each kind of JavaThread
+
 class Thread: public ThreadShadow {
   friend class VMStructs;
   friend class JVMCIVMStructs;
@@ -120,7 +141,6 @@ class Thread: public ThreadShadow {
   static THREAD_LOCAL_DECL Thread* _thr_current;
 #endif
 
- private:
   // Thread local data area available to the GC. The internal
   // structure and contents of this data area is GC-specific.
   // Only GC and GC barrier code should access this data area.
@@ -142,6 +162,9 @@ class Thread: public ThreadShadow {
   // const char* _exception_file;                   // file information for exception (debugging only)
   // int         _exception_line;                   // line information for exception (debugging only)
  protected:
+
+  DEBUG_ONLY(static Thread* _starting_thread;)
+
   // Support for forcing alignment of thread objects for biased locking
   void*       _real_malloc_address;
 
@@ -418,6 +441,21 @@ class Thread: public ThreadShadow {
  protected:
   // To be implemented by children.
   virtual void run() = 0;
+  virtual void pre_run() = 0;
+  virtual void post_run() = 0;  // Note: Thread must not be deleted prior to calling this!
+
+#ifdef ASSERT
+  enum RunState {
+    PRE_CALL_RUN,
+    CALL_RUN,
+    PRE_RUN,
+    RUN,
+    POST_RUN
+    // POST_CALL_RUN - can't define this one as 'this' may be deleted when we want to set it
+  };
+  RunState _run_state;  // for lifecycle checks
+#endif
+
 
  public:
   // invokes <ChildThreadClass>::run(), with common preparations and cleanups.
@@ -796,6 +834,13 @@ class NonJavaThread: public Thread {
   class List;
   static List _the_list;
 
+  void add_to_the_list();
+  void remove_from_the_list();
+
+ protected:
+  virtual void pre_run();
+  virtual void post_run();
+
  public:
   NonJavaThread();
   ~NonJavaThread();
@@ -803,12 +848,12 @@ class NonJavaThread: public Thread {
   class Iterator;
 };
 
-// Provides iteration over the list of NonJavaThreads.  Because list
-// management occurs in the NonJavaThread constructor and destructor,
-// entries in the list may not be fully constructed instances of a
-// derived class.  Threads created after an iterator is constructed
-// will not be visited by the iterator.  The scope of an iterator is a
-// critical section; there must be no safepoint checks in that scope.
+// Provides iteration over the list of NonJavaThreads.
+// List addition occurs in pre_run(), and removal occurs in post_run(),
+// so that only live fully-initialized threads can be found in the list.
+// Threads created after an iterator is constructed will not be visited
+// by the iterator. The scope of an iterator is a critical section; there
+// must be no safepoint checks in that scope.
 class NonJavaThread::Iterator : public StackObj {
   uint _protect_enter;
   NonJavaThread* _current;
@@ -844,7 +889,6 @@ class NamedThread: public NonJavaThread {
   ~NamedThread();
   // May only be called once per thread.
   void set_name(const char* format, ...)  ATTRIBUTE_PRINTF(2, 3);
-  void initialize_named_thread();
   virtual bool is_Named_thread() const { return true; }
   virtual char* name() const { return _name == NULL ? (char*)"Unknown Thread" : _name; }
   JavaThread *processed_thread() { return _processed_thread; }
@@ -875,7 +919,7 @@ class WorkerThread: public NamedThread {
 // A single WatcherThread is used for simulating timer interrupts.
 class WatcherThread: public NonJavaThread {
   friend class VMStructs;
- public:
+ protected:
   virtual void run();
 
  private:
@@ -1832,9 +1876,9 @@ class JavaThread: public Thread {
   void print_name_on_error(outputStream* st, char* buf, int buflen) const;
   void verify();
   const char* get_thread_name() const;
- private:
+ protected:
   // factor out low-level mechanics for use in both normal and error cases
-  const char* get_thread_name_string(char* buf = NULL, int buflen = 0) const;
+  virtual const char* get_thread_name_string(char* buf = NULL, int buflen = 0) const;
  public:
   const char* get_threadgroup_name() const;
   const char* get_parent_name() const;
@@ -1887,9 +1931,12 @@ class JavaThread: public Thread {
 
   inline CompilerThread* as_CompilerThread();
 
- public:
+ protected:
+  virtual void pre_run();
   virtual void run();
   void thread_main_inner();
+  virtual void post_run();
+
 
  private:
   GrowableArray<oop>* _array_for_gc;

src/hotspot/share/runtime/vmThread.cpp

@@ -285,8 +285,6 @@ void VMThread::destroy() {
 void VMThread::run() {
   assert(this == vm_thread(), "check");
 
-  this->initialize_named_thread();
-
   // Notify_lock wait checks on active_handles() to rewait in
   // case of spurious wakeup, it should wait on the last
   // value set prior to the notify

src/hotspot/share/services/management.cpp

@@ -1645,12 +1645,6 @@ void ThreadTimesClosure::do_thread(Thread* thread) {
     return;
   }
 
-  // NonJavaThread instances may not be fully initialized yet, so we need to
-  // skip any that aren't - check for zero stack_size()
-  if (!thread->is_Java_thread() && thread->stack_size() == 0) {
-    return;
-  }
-
   if (_count >= _names_len || _count >= _times_len) {
     // skip if the result array is not big enough
     return;

test/hotspot/gtest/threadHelper.inline.hpp

@@ -50,6 +50,9 @@ public:
   Semaphore _unblock;
   VMThreadBlocker() {}
   virtual ~VMThreadBlocker() {}
+  const char* get_thread_name_string(char* buf, int buflen) const {
+    return (char*) "VMThreadBlocker";
+  }
   void run() {
     this->set_thread_state(_thread_in_vm);
     {
@@ -58,9 +61,15 @@ public:
     }
     VM_StopSafepoint ss(&_ready, &_unblock);
     VMThread::execute(&ss);
+  }
+
+  // Override as JavaThread::post_run() calls JavaThread::exit which
+  // expects a valid thread object oop.
+  virtual void post_run() {
     Threads::remove(this);
     this->smr_delete();
   }
+
   void doit() {
     if (os::create_thread(this, os::os_thread)) {
       os::start_thread(this);
@@ -85,7 +94,11 @@ public:
   }
   virtual ~JavaTestThread() {}
 
-  void prerun() {
+  const char* get_thread_name_string(char* buf, int buflen) const {
+    return (char*) "JavaTestThread";
+  }
+
+  void pre_run() {
     this->set_thread_state(_thread_in_vm);
     {
       MutexLocker ml(Threads_lock);
@@ -99,12 +112,12 @@ public:
   virtual void main_run() = 0;
 
   void run() {
-    prerun();
     main_run();
-    postrun();
   }
 
-  void postrun() {
+  // Override as JavaThread::post_run() calls JavaThread::exit which
+  // expects a valid thread object oop. And we need to call signal.
+  void post_run() {
     Threads::remove(this);
     _post->signal();
     this->smr_delete();