diff --git a/runtime/vm/gc_sweeper.cc b/runtime/vm/gc_sweeper.cc index 2b4ee5822201..4cd224377b14 100644 --- a/runtime/vm/gc_sweeper.cc +++ b/runtime/vm/gc_sweeper.cc @@ -137,13 +137,14 @@ class SweeperTask : public ThreadPool::Task { if (page == last_) break; page = next_page; } + // Exit isolate cleanly *before* notifying it, to avoid shutdown race. + Thread::ExitIsolateAsHelper(); // This sweeper task is done. Notify the original isolate. { MonitorLocker ml(old_space_->tasks_lock()); old_space_->set_tasks(old_space_->tasks() - 1); ml.Notify(); } - Thread::ExitIsolateAsHelper(); } private: diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index b247e566b893..bd19791cf8e7 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -1437,6 +1437,15 @@ void Isolate::Shutdown() { // avoid exposing it in a state of decay. RemoveIsolateFromList(this); + if (heap_ != NULL) { + // Wait for any concurrent GC tasks to finish before shutting down. + PageSpace* old_space = heap_->old_space(); + MonitorLocker ml(old_space->tasks_lock()); + while (old_space->tasks() > 0) { + ml.Wait(); + } + } + // Create an area where we do have a zone and a handle scope so that we can // call VM functions while tearing this isolate down. { @@ -1490,6 +1499,8 @@ void Isolate::Shutdown() { // TODO(5411455): For now just make sure there are no current isolates // as we are shutting down the isolate. Thread::ExitIsolate(); + // All threads should have exited by now. + thread_registry()->CheckNotScheduled(this); Profiler::ShutdownProfilingForIsolate(this); } diff --git a/runtime/vm/os_thread_android.cc b/runtime/vm/os_thread_android.cc index 443e800e4d54..ae1b9f2d4e81 100644 --- a/runtime/vm/os_thread_android.cc +++ b/runtime/vm/os_thread_android.cc @@ -23,6 +23,14 @@ namespace dart { } +#if defined(DEBUG) +#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result) +#else +// NOTE: This (currently) expands to a no-op. +#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0) +#endif + + #ifdef DEBUG #define RETURN_ON_PTHREAD_FAILURE(result) \ if (result != 0) { \ @@ -217,7 +225,7 @@ void Mutex::Lock() { int result = pthread_mutex_lock(data_.mutex()); // Specifically check for dead lock to help debugging. ASSERT(result != EDEADLK); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -231,7 +239,7 @@ bool Mutex::TryLock() { if (result == EBUSY) { return false; } - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -249,7 +257,7 @@ void Mutex::Unlock() { int result = pthread_mutex_unlock(data_.mutex()); // Specifically check for wrong thread unlocking to aid debugging. ASSERT(result != EPERM); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. } diff --git a/runtime/vm/os_thread_linux.cc b/runtime/vm/os_thread_linux.cc index 5a0fe4fb4fb1..94bb82305451 100644 --- a/runtime/vm/os_thread_linux.cc +++ b/runtime/vm/os_thread_linux.cc @@ -24,6 +24,14 @@ namespace dart { } +#if defined(DEBUG) +#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result) +#else +// NOTE: This (currently) expands to a no-op. +#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0) +#endif + + #ifdef DEBUG #define RETURN_ON_PTHREAD_FAILURE(result) \ if (result != 0) { \ @@ -218,7 +226,7 @@ void Mutex::Lock() { int result = pthread_mutex_lock(data_.mutex()); // Specifically check for dead lock to help debugging. ASSERT(result != EDEADLK); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -232,7 +240,7 @@ bool Mutex::TryLock() { if (result == EBUSY) { return false; } - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -250,7 +258,7 @@ void Mutex::Unlock() { int result = pthread_mutex_unlock(data_.mutex()); // Specifically check for wrong thread unlocking to aid debugging. ASSERT(result != EPERM); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. } diff --git a/runtime/vm/os_thread_macos.cc b/runtime/vm/os_thread_macos.cc index 2b870db479d3..6a9aca22cd58 100644 --- a/runtime/vm/os_thread_macos.cc +++ b/runtime/vm/os_thread_macos.cc @@ -31,6 +31,14 @@ namespace dart { } +#if defined(DEBUG) +#define ASSERT_PTHREAD_SUCCESS(result) VALIDATE_PTHREAD_RESULT(result) +#else +// NOTE: This (currently) expands to a no-op. +#define ASSERT_PTHREAD_SUCCESS(result) ASSERT(result == 0) +#endif + + #ifdef DEBUG #define RETURN_ON_PTHREAD_FAILURE(result) \ if (result != 0) { \ @@ -223,7 +231,7 @@ void Mutex::Lock() { int result = pthread_mutex_lock(data_.mutex()); // Specifically check for dead lock to help debugging. ASSERT(result != EDEADLK); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -237,7 +245,7 @@ bool Mutex::TryLock() { if ((result == EBUSY) || (result == EDEADLK)) { return false; } - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. // When running with assertions enabled we do track the owner. #if defined(DEBUG) owner_ = OSThread::GetCurrentThreadId(); @@ -255,7 +263,7 @@ void Mutex::Unlock() { int result = pthread_mutex_unlock(data_.mutex()); // Specifically check for wrong thread unlocking to aid debugging. ASSERT(result != EPERM); - ASSERT(result == 0); // Verify no other errors. + ASSERT_PTHREAD_SUCCESS(result); // Verify no other errors. } diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index a82c8b3a358d..bbdd1b401859 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -27,6 +27,12 @@ static void DeleteThread(void* thread) { } +Thread::~Thread() { + // We should cleanly exit any isolate before destruction. + ASSERT(isolate_ == NULL); +} + + void Thread::InitOnceBeforeIsolate() { ASSERT(thread_key_ == OSThread::kUnsetThreadLocalKey); thread_key_ = OSThread::CreateThreadLocal(DeleteThread); diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index 00cea34611a3..bc9cb61dcd69 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -75,6 +75,8 @@ class Thread { static void InitOnceBeforeIsolate(); static void InitOnceAfterObjectAndStubCode(); + ~Thread(); + // The topmost zone used for allocation in this thread. Zone* zone() const { return state_.zone; } diff --git a/runtime/vm/thread_registry.h b/runtime/vm/thread_registry.h index d4de8a3fa469..af160269fa33 100644 --- a/runtime/vm/thread_registry.h +++ b/runtime/vm/thread_registry.h @@ -63,6 +63,19 @@ class ThreadRegistry { return (FindEntry(thread) != NULL); } + void CheckNotScheduled(Isolate* isolate) { + MutexLocker ml(mutex_); + for (int i = 0; i < entries_.length(); ++i) { + const Entry& entry = entries_[i]; + if (entry.scheduled) { + FATAL3("Isolate %p still scheduled on %p (whose isolate_ is %p)\n", + isolate, + entry.thread, + entry.thread->isolate()); + } + } + } + void VisitObjectPointers(ObjectPointerVisitor* visitor) { MutexLocker ml(mutex_); for (int i = 0; i < entries_.length(); ++i) {