Skip to content

Commit f017fe7

Browse files
chinmaygardechinmaygarde
authored
Avoid manually shutting down engine managed isolates. (flutter#8621)
These are now shutdown by the VM and cleanup waits for their shutdown.
1 parent 80e934e commit f017fe7

File tree

6 files changed

+40
-102
lines changed

6 files changed

+40
-102
lines changed

‎BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ group("flutter") {
4747
public_deps += [
4848
"$flutter_root/flow:flow_unittests",
4949
"$flutter_root/fml:fml_unittests",
50+
"$flutter_root/runtime:runtime_lifecycle_unittests",
5051
"$flutter_root/runtime:runtime_unittests",
5152
"$flutter_root/shell/common:shell_unittests",
5253
"$flutter_root/shell/platform/common/cpp/client_wrapper:client_wrapper_unittests",

‎runtime/dart_isolate.cc

-4
Original file line numberDiff line numberDiff line change
@@ -721,8 +721,6 @@ DartIsolate::CreateDartVMAndEmbedderObjectPair(
721721
}
722722
}
723723

724-
DartVMRef::GetRunningVM()->RegisterActiveIsolate(*embedder_isolate);
725-
726724
// The ownership of the embedder object is controlled by the Dart VM. So the
727725
// only reference returned to the caller is weak.
728726
embedder_isolate.release();
@@ -760,8 +758,6 @@ void DartIsolate::AddIsolateShutdownCallback(fml::closure closure) {
760758

761759
void DartIsolate::OnShutdownCallback() {
762760
shutdown_callbacks_.clear();
763-
DartVMRef::GetRunningVM()->UnregisterActiveIsolate(
764-
std::static_pointer_cast<DartIsolate>(shared_from_this()));
765761
}
766762

767763
DartIsolate::AutoFireClosure::AutoFireClosure(fml::closure closure)

‎runtime/dart_lifecycle_unittests.cc

+36-35
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include "flutter/common/task_runners.h"
66
#include "flutter/fml/paths.h"
7+
#include "flutter/fml/synchronization/count_down_latch.h"
78
#include "flutter/fml/synchronization/waitable_event.h"
89
#include "flutter/runtime/dart_vm.h"
910
#include "flutter/runtime/dart_vm_lifecycle.h"
@@ -41,8 +42,7 @@ TEST_F(DartLifecycleTest, CanStartAndShutdownVMOverAndOver) {
4142
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
4243
}
4344

44-
static void CreateAndRunRootIsolate(
45-
std::shared_ptr<DartIsolate>& isolate_result,
45+
static std::shared_ptr<DartIsolate> CreateAndRunRootIsolate(
4646
const Settings& settings,
4747
const DartVMData& vm,
4848
fml::RefPtr<fml::TaskRunner> task_runner,
@@ -67,73 +67,74 @@ static void CreateAndRunRootIsolate(
6767

6868
if (!isolate) {
6969
FML_LOG(ERROR) << "Could not create valid isolate.";
70-
return;
70+
return nullptr;
7171
}
7272

7373
if (DartVM::IsRunningPrecompiledCode()) {
7474
if (!isolate->PrepareForRunningFromPrecompiledCode()) {
7575
FML_LOG(ERROR)
7676
<< "Could not prepare to run the isolate from precompiled code.";
77-
return;
77+
return nullptr;
7878
}
7979

8080
} else {
8181
if (!isolate->PrepareForRunningFromKernels(
8282
settings.application_kernels())) {
8383
FML_LOG(ERROR) << "Could not prepare isolate from application kernels.";
84-
return;
84+
return nullptr;
8585
}
8686
}
8787

8888
if (isolate->GetPhase() != DartIsolate::Phase::Ready) {
8989
FML_LOG(ERROR) << "Isolate was not ready.";
90-
return;
90+
return nullptr;
9191
}
9292

9393
if (!isolate->Run(entrypoint, settings.root_isolate_create_callback)) {
9494
FML_LOG(ERROR) << "Could not run entrypoint: " << entrypoint << ".";
95-
return;
95+
return nullptr;
9696
}
9797

9898
if (isolate->GetPhase() != DartIsolate::Phase::Running) {
9999
FML_LOG(ERROR) << "Isolate was not Running.";
100-
return;
100+
return nullptr;
101101
}
102102

103-
isolate_result = isolate;
104-
}
105-
106-
static std::shared_ptr<DartIsolate> CreateAndRunRootIsolate(
107-
const Settings& settings,
108-
const DartVMData& vm,
109-
fml::RefPtr<fml::TaskRunner> task_runner,
110-
std::string entrypoint) {
111-
fml::AutoResetWaitableEvent latch;
112-
std::shared_ptr<DartIsolate> isolate;
113-
fml::TaskRunner::RunNowOrPostTask(task_runner, [&]() {
114-
CreateAndRunRootIsolate(isolate, settings, vm, task_runner, entrypoint);
115-
latch.Signal();
116-
});
117-
latch.Wait();
118103
return isolate;
119104
}
120105

121-
TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownTheIsolate) {
106+
TEST_F(DartLifecycleTest, ShuttingDownTheVMShutsDownAllIsolates) {
122107
auto settings = CreateSettingsForFixture();
123108
settings.leak_vm = false;
124-
settings.enable_observatory = false;
125-
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
126-
{
109+
// Make sure the service protocol launches
110+
settings.enable_observatory = true;
111+
112+
for (size_t i = 0; i < 3; i++) {
113+
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
114+
115+
const auto last_launch_count = DartVM::GetVMLaunchCount();
116+
127117
auto vm_ref = DartVMRef::Create(settings);
118+
128119
ASSERT_TRUE(DartVMRef::IsInstanceRunning());
129-
ASSERT_EQ(vm_ref->GetIsolateCount(), 0u);
130-
auto isolate =
131-
CreateAndRunRootIsolate(settings, *vm_ref.GetVMData(),
132-
GetThreadTaskRunner(), "testIsolateShutdown");
133-
ASSERT_TRUE(isolate);
134-
ASSERT_EQ(vm_ref->GetIsolateCount(), 1u);
135-
vm_ref->ShutdownAllIsolates();
136-
ASSERT_EQ(vm_ref->GetIsolateCount(), 0u);
120+
ASSERT_EQ(last_launch_count + 1, DartVM::GetVMLaunchCount());
121+
122+
const size_t isolate_count = 100;
123+
124+
fml::CountDownLatch latch(isolate_count);
125+
auto vm_data = vm_ref.GetVMData();
126+
auto thread_task_runner = GetThreadTaskRunner();
127+
for (size_t i = 0; i < isolate_count; ++i) {
128+
thread_task_runner->PostTask(
129+
[vm_data, &settings, &latch, thread_task_runner]() {
130+
ASSERT_TRUE(CreateAndRunRootIsolate(settings, *vm_data.get(),
131+
thread_task_runner,
132+
"testIsolateShutdown"));
133+
latch.CountDown();
134+
});
135+
}
136+
137+
latch.Wait();
137138
}
138139
ASSERT_FALSE(DartVMRef::IsInstanceRunning());
139140
}

‎runtime/dart_vm.cc

-50
Original file line numberDiff line numberDiff line change
@@ -460,54 +460,4 @@ std::shared_ptr<IsolateNameServer> DartVM::GetIsolateNameServer() const {
460460
return isolate_name_server_;
461461
}
462462

463-
size_t DartVM::GetIsolateCount() const {
464-
std::lock_guard<std::mutex> lock(active_isolates_mutex_);
465-
return active_isolates_.size();
466-
}
467-
468-
void DartVM::ShutdownAllIsolates() {
469-
std::set<std::shared_ptr<DartIsolate>> isolates_to_shutdown;
470-
// We may be shutting down isolates on the current thread. Shutting down the
471-
// isolate calls the shutdown callback which removes the entry from the
472-
// active isolate. The lock must be obtained to mutate that entry. To avoid a
473-
// deadlock, collect the isolate is a seprate collection.
474-
{
475-
std::lock_guard<std::mutex> lock(active_isolates_mutex_);
476-
for (const auto& active_isolate : active_isolates_) {
477-
if (auto task_runner = active_isolate->GetMessageHandlingTaskRunner()) {
478-
isolates_to_shutdown.insert(active_isolate);
479-
}
480-
}
481-
}
482-
483-
fml::CountDownLatch latch(isolates_to_shutdown.size());
484-
485-
for (const auto& isolate : isolates_to_shutdown) {
486-
fml::TaskRunner::RunNowOrPostTask(
487-
isolate->GetMessageHandlingTaskRunner(), [&latch, isolate]() {
488-
if (!isolate || !isolate->Shutdown()) {
489-
FML_LOG(ERROR) << "Could not shutdown isolate.";
490-
}
491-
latch.CountDown();
492-
});
493-
}
494-
latch.Wait();
495-
}
496-
497-
void DartVM::RegisterActiveIsolate(std::shared_ptr<DartIsolate> isolate) {
498-
if (!isolate) {
499-
return;
500-
}
501-
std::lock_guard<std::mutex> lock(active_isolates_mutex_);
502-
active_isolates_.insert(isolate);
503-
}
504-
505-
void DartVM::UnregisterActiveIsolate(std::shared_ptr<DartIsolate> isolate) {
506-
if (!isolate) {
507-
return;
508-
}
509-
std::lock_guard<std::mutex> lock(active_isolates_mutex_);
510-
active_isolates_.erase(isolate);
511-
}
512-
513463
} // namespace flutter

‎runtime/dart_vm.h

-13
Original file line numberDiff line numberDiff line change
@@ -36,24 +36,15 @@ class DartVM {
3636

3737
std::shared_ptr<const DartVMData> GetVMData() const;
3838

39-
// This accessor is racy and only meant to the used in tests where there is a
40-
// consistent threading mode.
41-
size_t GetIsolateCount() const;
42-
4339
std::shared_ptr<ServiceProtocol> GetServiceProtocol() const;
4440

4541
std::shared_ptr<IsolateNameServer> GetIsolateNameServer() const;
4642

47-
void ShutdownAllIsolates();
48-
4943
private:
5044
const Settings settings_;
5145
std::shared_ptr<const DartVMData> vm_data_;
5246
const std::shared_ptr<IsolateNameServer> isolate_name_server_;
5347
const std::shared_ptr<ServiceProtocol> service_protocol_;
54-
mutable std::mutex active_isolates_mutex_;
55-
std::set<std::shared_ptr<DartIsolate>> active_isolates_
56-
FML_GUARDED_BY(active_isolates_mutex_);
5748

5849
friend class DartVMRef;
5950
friend class DartIsolate;
@@ -68,10 +59,6 @@ class DartVM {
6859
DartVM(std::shared_ptr<const DartVMData> data,
6960
std::shared_ptr<IsolateNameServer> isolate_name_server);
7061

71-
void RegisterActiveIsolate(std::shared_ptr<DartIsolate> isolate);
72-
73-
void UnregisterActiveIsolate(std::shared_ptr<DartIsolate> isolate);
74-
7562
FML_DISALLOW_COPY_AND_ASSIGN(DartVM);
7663
};
7764

‎testing/run_tests.sh

+3
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ echo "Running fml_unittests..."
3535
echo "Running runtime_unittests..."
3636
"$HOST_DIR/runtime_unittests"
3737

38+
echo "Running runtime_lifecycle_unittests..."
39+
"$HOST_DIR/runtime_lifecycle_unittests"
40+
3841
echo "Running shell_unittests..."
3942
"$HOST_DIR/shell_unittests"
4043

0 commit comments

Comments
 (0)