Completely remove InterruptableThreadState and Fix ThreadRegistry leak

runtime/vm/thread.cc

Index: runtime/vm/thread.cc
diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc
index fdd3272e340ecf1029b9a87c5c067bbcd3b80440..b3d93e3acbd9e262fd1600303e0b8e10eb0c13eb 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -21,6 +21,24 @@ namespace dart {
 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);
 }
@@ -29,6 +47,9 @@ static void DeleteThread(void* 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);
 }
 
 
@@ -74,7 +95,8 @@ void Thread::CleanUp() {
 
 
 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)    \
@@ -131,15 +153,6 @@ void Thread::EnterIsolate(Isolate* isolate) {
   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);
 }
@@ -151,7 +164,6 @@ void Thread::ExitIsolate() {
   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();
@@ -259,6 +271,19 @@ void Thread::set_cha(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;