Completely remove InterruptableThreadState and Fix ThreadRegistry leak

runtime/vm/thread.cc

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/thread.h"
 
 #include "vm/growable_array.h"
 #include "vm/isolate.h"
 #include "vm/lockers.h"
 #include "vm/object.h"
 #include "vm/os_thread.h"
 #include "vm/profiler.h"
 #include "vm/stub_code.h"
 #include "vm/thread_interrupter.h"
 #include "vm/thread_registry.h"
 
 namespace dart {
 
 // The single thread local key which stores all the thread local data
 // for a thread.
 ThreadLocalKey Thread::thread_key_ = OSThread::kUnsetThreadLocalKey;
 
 
+// Remove |thread| from all isolate's thread registry.

[koda, 2015/08/20 14:55:53]

(I'm not a native speaker, but shouldn't this be either "each isolate's
registry" or "all isolates' registries"?)

[Cutch, 2015/08/20 20:40:19]

Done.

+class ThreadPruner : public IsolateVisitor {
+ public:
+  explicit ThreadPruner(Thread* thread)
+      : thread_(thread) {
+    ASSERT(thread_ != NULL);
+  }
+
+  void VisitIsolate(Isolate* isolate) {
+    ThreadRegistry* registry = isolate->thread_registry();
+    ASSERT(registry != NULL);
+    registry->PruneThread(thread_);
+  }
+ private:
+  Thread* thread_;
+};
+
+
 static void DeleteThread(void* thread) {
   delete reinterpret_cast<Thread*>(thread);
 }
 
 
 Thread::~Thread() {
   // We should cleanly exit any isolate before destruction.
   ASSERT(isolate_ == NULL);
+  // Clear |this| from all isolate's thread registry.
+  ThreadPruner pruner(this);
+  Isolate::VisitIsolates(&pruner);
 }
 
 
 void Thread::InitOnceBeforeIsolate() {
   ASSERT(thread_key_ == OSThread::kUnsetThreadLocalKey);
   thread_key_ = OSThread::CreateThreadLocal(DeleteThread);
   ASSERT(thread_key_ != OSThread::kUnsetThreadLocalKey);
   ASSERT(Thread::Current() == NULL);
   // Postpone initialization of VM constants for this first thread.
   SetCurrent(new Thread(false));
@@ skipping 25 matching lines @@
   Thread* current = Current();
   if (current != NULL) {
     delete current;
   }
   SetCurrent(NULL);
 }
 #endif
 
 
 Thread::Thread(bool init_vm_constants)
-    : isolate_(NULL),
+    : id_(OSThread::GetCurrentThreadId()),

[koda, 2015/08/20 14:55:53]

This should be in the same place as the call to SetCurrent, to make sure the TLS
matches the id_.

Consider initializing id_ to the invalid id here and then update it in
SetCurrent (or, possibly, move the SetCurrent to the end of this constructor,
although having such a side-effect might be surprising/problematic, even if
documented).

[Cutch, 2015/08/20 20:40:19]

As discussed. I've moved the SetCurrent call into the constructor and the thread
id is const.

+      isolate_(NULL),
       store_buffer_block_(NULL) {
   ClearState();
 #define DEFAULT_INIT(type_name, member_name, init_expr, default_init_value)    \
   member_name = default_init_value;
 CACHED_CONSTANTS_LIST(DEFAULT_INIT)
 #undef DEFAULT_INIT
   if (init_vm_constants) {
     InitVMConstants();
   }
 }
@@ skipping 36 matching lines @@
   ASSERT(thread->isolate() == NULL);
   ASSERT(!isolate->HasMutatorThread());
   thread->isolate_ = isolate;
   isolate->MakeCurrentThreadMutator(thread);
   isolate->set_vm_tag(VMTag::kVMTagId);
   ASSERT(thread->store_buffer_block_ == NULL);
   thread->StoreBufferAcquire();
   ASSERT(isolate->heap() != NULL);
   thread->heap_ = isolate->heap();
   thread->Schedule(isolate);
-  ASSERT(thread->thread_state() == NULL);
-  InterruptableThreadState* thread_state =
-      ThreadInterrupter::GetCurrentThreadState();
-#if defined(DEBUG)
-  thread->set_thread_state(NULL);  // Exclude thread itself from the dupe check.
-  Isolate::CheckForDuplicateThreadState(thread_state);
-  thread->set_thread_state(thread_state);
-#endif
-  ASSERT(thread_state != NULL);
   // TODO(koda): Migrate profiler interface to use Thread.
   Profiler::BeginExecution(isolate);
 }
 
 
 void Thread::ExitIsolate() {
   Thread* thread = Thread::Current();
   // TODO(koda): Audit callers; they should know whether they're in an isolate.
   if (thread == NULL || thread->isolate() == NULL) return;
   Isolate* isolate = thread->isolate();
   Profiler::EndExecution(isolate);
-  thread->set_thread_state(NULL);
   thread->Unschedule();
   // TODO(koda): Move store_buffer_block_ into State.
   thread->StoreBufferRelease();
   if (isolate->is_runnable()) {
     isolate->set_vm_tag(VMTag::kIdleTagId);
   } else {
     isolate->set_vm_tag(VMTag::kLoadWaitTagId);
   }
   isolate->ClearMutatorThread();
   thread->isolate_ = NULL;
@@ skipping 87 matching lines @@
   return isolate_->cha_;
 }
 
 
 void Thread::set_cha(CHA* value) {
   ASSERT(isolate_ != NULL);
   isolate_->cha_ = value;
 }
 
 
+void Thread::SetThreadInterrupter(ThreadInterruptCallback callback,

[koda, 2015/08/20 15:45:14]

Consider ASSERT that this is only called on the current thread.

[Cutch, 2015/08/20 20:40:18]

Done.

+                                  void* data) {
+  thread_interrupt_callback_ = callback;

[koda, 2015/08/20 14:55:53]

This update is not done atomically. Thus, if we get a signal while the update
(triggered by enter/exit isolate) is partially complete, the callback/data might
be in an inconsistent state.

Previously, this was avoided (in practice, albeit not in theory) by temporarily
clearing the ITS* while updating.

We need to think through how to make this as safe as possible without expensive
locking.

[koda, 2015/08/20 15:18:33]

One way to make the interface safer is to remove the unsynchronized accessors to
the individual fields and instead change ShouldInterrupt to:

1. Also take two output arguments (i.e., a ThreadInterruptCallback* and a
void**), and
2. Read the fields as volatiles (they should probably be declared volatile in
the class)
3. Only set the results and return true if both are non-NULL

And 4. Actually use ShouldInterrupt; it's currently unused in your CL.

(Actually, 2 is probably not strictly needed in our current implementation,
since, as iposva pointed out, the thread is either paused or the current thread,
so the values won't change beneath you. But assuming this will always be the
case is fragile.)

[Cutch, 2015/08/20 20:40:18]

I've removed the individual accessors. New API:

bool IsThreadInterrupterEnabled(ThreadInterruptCallback* callback, void** data)
const;

+  thread_interrupt_data_ = data;
+}
+
+
+bool Thread::ShouldInterrupt() const {

[koda, 2015/08/20 15:45:14]

Consider ASSERT that this is only called on the current thread unless we're on
Windows.

[Cutch, 2015/08/20 20:40:19]

Done.

+  return (thread_interrupt_data_ != NULL) &&
+         (thread_interrupt_callback_ != NULL);
+}
+
+
 bool Thread::CanLoadFromThread(const Object& object) {
 #define CHECK_OBJECT(type_name, member_name, expr, default_init_value)         \
   if (object.raw() == expr) return true;
 CACHED_VM_OBJECTS_LIST(CHECK_OBJECT)
 #undef CHECK_OBJECT
   return false;
 }
 
 
 intptr_t Thread::OffsetFromThread(const Object& object) {
 #define COMPUTE_OFFSET(type_name, member_name, expr, default_init_value)       \
   ASSERT((expr)->IsVMHeapObject());                                            \
   if (object.raw() == expr) return Thread::member_name##offset();
 CACHED_VM_OBJECTS_LIST(COMPUTE_OFFSET)
 #undef COMPUTE_OFFSET
   UNREACHABLE();
   return -1;
 }
 
 }  // namespace dart