From f017fe74aa78ee5465e6a8c55d23fcde1e4d2f74 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Wed, 17 Apr 2019 16:11:47 -0700 Subject: [PATCH] Avoid manually shutting down engine managed isolates. (#8621) These are now shutdown by the VM and cleanup waits for their shutdown. --- BUILD.gn | 1 + runtime/dart_isolate.cc | 4 -- runtime/dart_lifecycle_unittests.cc | 71 +++++++++++++++-------------- runtime/dart_vm.cc | 50 -------------------- runtime/dart_vm.h | 13 ------ testing/run_tests.sh | 3 ++ 6 files changed, 40 insertions(+), 102 deletions(-) diff --git a/BUILD.gn b/BUILD.gn index 1ae4a19c648..a3c33e6fef6 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -47,6 +47,7 @@ group("flutter") { public_deps += [ "$flutter_root/flow:flow_unittests", "$flutter_root/fml:fml_unittests", + "$flutter_root/runtime:runtime_lifecycle_unittests", "$flutter_root/runtime:runtime_unittests", "$flutter_root/shell/common:shell_unittests", "$flutter_root/shell/platform/common/cpp/client_wrapper:client_wrapper_unittests", diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index 3c888887d98..20b7c960ee2 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -721,8 +721,6 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair( } } - DartVMRef::GetRunningVM()->RegisterActiveIsolate(*embedder_isolate); - // The ownership of the embedder object is controlled by the Dart VM. So the // only reference returned to the caller is weak. embedder_isolate.release(); @@ -760,8 +758,6 @@ void DartIsolate::AddIsolateShutdownCallback(fml::closure closure) { void DartIsolate::OnShutdownCallback() { shutdown_callbacks_.clear(); - DartVMRef::GetRunningVM()->UnregisterActiveIsolate( - std::static_pointer_cast(shared_from_this())); } DartIsolate::AutoFireClosure::AutoFireClosure(fml::closure closure) diff --git a/runtime/dart_lifecycle_unittests.cc b/runtime/dart_lifecycle_unittests.cc index b51734deb1c..f50de290890 100644 --- a/runtime/dart_lifecycle_unittests.cc +++ b/runtime/dart_lifecycle_unittests.cc @@ -4,6 +4,7 @@ #include "flutter/common/task_runners.h" #include "flutter/fml/paths.h" +#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/fml/synchronization/waitable_event.h" #include "flutter/runtime/dart_vm.h" #include "flutter/runtime/dart_vm_lifecycle.h" @@ -41,8 +42,7 @@ TEST_F(DartLifecycleTest, CanStartAndShutdownVMOverAndOver) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -static void CreateAndRunRootIsolate( - std::shared_ptr& isolate_result, +static std::shared_ptr CreateAndRunRootIsolate( const Settings& settings, const DartVMData& vm, fml::RefPtr task_runner, @@ -67,73 +67,74 @@ static void CreateAndRunRootIsolate( if (!isolate) { FML_LOG(ERROR) << "Could not create valid isolate."; - return; + return nullptr; } if (DartVM::IsRunningPrecompiledCode()) { if (!isolate->PrepareForRunningFromPrecompiledCode()) { FML_LOG(ERROR) << "Could not prepare to run the isolate from precompiled code."; - return; + return nullptr; } } else { if (!isolate->PrepareForRunningFromKernels( settings.application_kernels())) { FML_LOG(ERROR) << "Could not prepare isolate from application kernels."; - return; + return nullptr; } } if (isolate->GetPhase() != DartIsolate::Phase::Ready) { FML_LOG(ERROR) << "Isolate was not ready."; - return; + return nullptr; } if (!isolate->Run(entrypoint, settings.root_isolate_create_callback)) { FML_LOG(ERROR) << "Could not run entrypoint: " << entrypoint << "."; - return; + return nullptr; } if (isolate->GetPhase() != DartIsolate::Phase::Running) { FML_LOG(ERROR) << "Isolate was not Running."; - return; + return nullptr; } - isolate_result = isolate; -} - -static std::shared_ptr CreateAndRunRootIsolate( - const Settings& settings, - const DartVMData& vm, - fml::RefPtr task_runner, - std::string entrypoint) { - fml::AutoResetWaitableEvent latch; - std::shared_ptr isolate; - fml::TaskRunner::RunNowOrPostTask(task_runner, [&]() { - CreateAndRunRootIsolate(isolate, settings, vm, task_runner, entrypoint); - latch.Signal(); - }); - latch.Wait(); return isolate; } -TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownTheIsolate) { +TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownAllIsolates) { auto settings = CreateSettingsForFixture(); settings.leak_vm = false; - settings.enable_observatory = false; - ASSERT_FALSE(DartVMRef::IsInstanceRunning()); - { + // Make sure the service protocol launches + settings.enable_observatory = true; + + for (size_t i = 0; i < 3; i++) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + + const auto last_launch_count = DartVM::GetVMLaunchCount(); + auto vm_ref = DartVMRef::Create(settings); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); - ASSERT_EQ(vm_ref->GetIsolateCount(), 0u); - auto isolate = - CreateAndRunRootIsolate(settings, *vm_ref.GetVMData(), - GetThreadTaskRunner(), "testIsolateShutdown"); - ASSERT_TRUE(isolate); - ASSERT_EQ(vm_ref->GetIsolateCount(), 1u); - vm_ref->ShutdownAllIsolates(); - ASSERT_EQ(vm_ref->GetIsolateCount(), 0u); + ASSERT_EQ(last_launch_count + 1, DartVM::GetVMLaunchCount()); + + const size_t isolate_count = 100; + + fml::CountDownLatch latch(isolate_count); + auto vm_data = vm_ref.GetVMData(); + auto thread_task_runner = GetThreadTaskRunner(); + for (size_t i = 0; i < isolate_count; ++i) { + thread_task_runner->PostTask( + [vm_data, &settings, &latch, thread_task_runner]() { + ASSERT_TRUE(CreateAndRunRootIsolate(settings, *vm_data.get(), + thread_task_runner, + "testIsolateShutdown")); + latch.CountDown(); + }); + } + + latch.Wait(); } ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } diff --git a/runtime/dart_vm.cc b/runtime/dart_vm.cc index 476d05f2fc6..973d5d8e86e 100644 --- a/runtime/dart_vm.cc +++ b/runtime/dart_vm.cc @@ -460,54 +460,4 @@ std::shared_ptr DartVM::GetIsolateNameServer() const { return isolate_name_server_; } -size_t DartVM::GetIsolateCount() const { - std::lock_guard lock(active_isolates_mutex_); - return active_isolates_.size(); -} - -void DartVM::ShutdownAllIsolates() { - std::set> isolates_to_shutdown; - // We may be shutting down isolates on the current thread. Shutting down the - // isolate calls the shutdown callback which removes the entry from the - // active isolate. The lock must be obtained to mutate that entry. To avoid a - // deadlock, collect the isolate is a seprate collection. - { - std::lock_guard lock(active_isolates_mutex_); - for (const auto& active_isolate : active_isolates_) { - if (auto task_runner = active_isolate->GetMessageHandlingTaskRunner()) { - isolates_to_shutdown.insert(active_isolate); - } - } - } - - fml::CountDownLatch latch(isolates_to_shutdown.size()); - - for (const auto& isolate : isolates_to_shutdown) { - fml::TaskRunner::RunNowOrPostTask( - isolate->GetMessageHandlingTaskRunner(), [&latch, isolate]() { - if (!isolate || !isolate->Shutdown()) { - FML_LOG(ERROR) << "Could not shutdown isolate."; - } - latch.CountDown(); - }); - } - latch.Wait(); -} - -void DartVM::RegisterActiveIsolate(std::shared_ptr isolate) { - if (!isolate) { - return; - } - std::lock_guard lock(active_isolates_mutex_); - active_isolates_.insert(isolate); -} - -void DartVM::UnregisterActiveIsolate(std::shared_ptr isolate) { - if (!isolate) { - return; - } - std::lock_guard lock(active_isolates_mutex_); - active_isolates_.erase(isolate); -} - } // namespace flutter diff --git a/runtime/dart_vm.h b/runtime/dart_vm.h index 690130984a5..b88c033b6ef 100644 --- a/runtime/dart_vm.h +++ b/runtime/dart_vm.h @@ -36,24 +36,15 @@ class DartVM { std::shared_ptr GetVMData() const; - // This accessor is racy and only meant to the used in tests where there is a - // consistent threading mode. - size_t GetIsolateCount() const; - std::shared_ptr GetServiceProtocol() const; std::shared_ptr GetIsolateNameServer() const; - void ShutdownAllIsolates(); - private: const Settings settings_; std::shared_ptr vm_data_; const std::shared_ptr isolate_name_server_; const std::shared_ptr service_protocol_; - mutable std::mutex active_isolates_mutex_; - std::set> active_isolates_ - FML_GUARDED_BY(active_isolates_mutex_); friend class DartVMRef; friend class DartIsolate; @@ -68,10 +59,6 @@ class DartVM { DartVM(std::shared_ptr data, std::shared_ptr isolate_name_server); - void RegisterActiveIsolate(std::shared_ptr isolate); - - void UnregisterActiveIsolate(std::shared_ptr isolate); - FML_DISALLOW_COPY_AND_ASSIGN(DartVM); }; diff --git a/testing/run_tests.sh b/testing/run_tests.sh index 621743aa809..87f68897055 100755 --- a/testing/run_tests.sh +++ b/testing/run_tests.sh @@ -35,6 +35,9 @@ echo "Running fml_unittests..." echo "Running runtime_unittests..." "$HOST_DIR/runtime_unittests" +echo "Running runtime_lifecycle_unittests..." +"$HOST_DIR/runtime_lifecycle_unittests" + echo "Running shell_unittests..." "$HOST_DIR/shell_unittests"