Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Commit

Permalink
Fix some races in the platform isolate tests (#51358)
Browse files Browse the repository at this point in the history 
* Do not check IsolateIsShutdown after CreateAndRegisterIsolate in the MultithreadedCreation test.

The test's main thread could run ShutdownPlatformIsolates between a worker thread's calls to CreateAndRegisterIsolate and IsolateIsShutdown.  If that happens, IsolateIsShutdown will return true even though CreateAndRegisterIsolate succeeded.

* Change the is_registered flag to was_registered and do not clear it during isolate shutdown

Tests like MultithreadedCreation need to know whether RegisterPlatformIsolate succeeded so they can determine if the isolate will be shut down by ShutdownPlatformIsolates or if it needs to be shut down explicitly.  If the flag is cleared during shutdown, then the test may see an isolate that was successfully registered and shut down and think that it was never registered.
  • Loading branch information
@jason-simmons
jason-simmons authored Mar 12, 2024
1 parent e3f3c02 commit ea848c3
Showing 1 changed file with 12 additions and 22 deletions.
34 changes: 12 additions & 22 deletions runtime/platform_isolate_manager_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ struct IsolateData {
PlatformIsolateManager* mgr;
Dart_Isolate isolate = nullptr;
bool is_shutdown = false;
bool is_registered = false;
bool was_registered = false;
explicit IsolateData(PlatformIsolateManager* _mgr) : mgr(_mgr) {}
};

Expand Down Expand Up @@ -96,7 +96,7 @@ class PlatformIsolateManagerTest : public FixtureTest {

(*isolate_data_map_.get())[isolate].push_back(
std::unique_ptr<IsolateData>(isolate_data));
isolate_data->is_registered = mgr->RegisterPlatformIsolate(isolate);
isolate_data->was_registered = mgr->RegisterPlatformIsolate(isolate);

return isolate;
}
Expand All @@ -107,10 +107,10 @@ class PlatformIsolateManagerTest : public FixtureTest {
return (*isolate_data_map_.get())[isolate].back()->is_shutdown;
}

bool IsolateIsRegistered(Dart_Isolate isolate) {
bool IsolateWasRegistered(Dart_Isolate isolate) {
EXPECT_EQ(1u, isolate_data_map_.get()->count(isolate));
EXPECT_LT(0u, (*isolate_data_map_.get())[isolate].size());
return (*isolate_data_map_.get())[isolate].back()->is_registered;
return (*isolate_data_map_.get())[isolate].back()->was_registered;
}

private:
Expand All @@ -122,9 +122,8 @@ class PlatformIsolateManagerTest : public FixtureTest {
EXPECT_TRUE(isolate_data->isolate);
EXPECT_FALSE(isolate_data->is_shutdown);
isolate_data->is_shutdown = true;
if (isolate_data->is_registered) {
if (isolate_data->was_registered) {
isolate_data->mgr->RemovePlatformIsolate(isolate_data->isolate);
isolate_data->is_registered = false;
}
}

Expand All @@ -140,24 +139,22 @@ TEST_F(PlatformIsolateManagerTest, OrdinaryFlow) {
Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateA);
EXPECT_FALSE(IsolateIsShutdown(isolateA));
EXPECT_TRUE(IsolateIsRegistered(isolateA));
EXPECT_TRUE(IsolateWasRegistered(isolateA));
EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA));

Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateB);
EXPECT_FALSE(IsolateIsShutdown(isolateB));
EXPECT_TRUE(IsolateIsRegistered(isolateB));
EXPECT_TRUE(IsolateWasRegistered(isolateB));
EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateB));

mgr.ShutdownPlatformIsolates();
EXPECT_TRUE(mgr.HasShutdown());
EXPECT_TRUE(mgr.HasShutdownMaybeFalseNegative());

EXPECT_TRUE(IsolateIsShutdown(isolateA));
EXPECT_FALSE(IsolateIsRegistered(isolateA));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA));
EXPECT_TRUE(IsolateIsShutdown(isolateB));
EXPECT_FALSE(IsolateIsRegistered(isolateB));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB));
});
}
Expand All @@ -170,35 +167,31 @@ TEST_F(PlatformIsolateManagerTest, EarlyShutdown) {
Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateA);
EXPECT_FALSE(IsolateIsShutdown(isolateA));
EXPECT_TRUE(IsolateIsRegistered(isolateA));
EXPECT_TRUE(IsolateWasRegistered(isolateA));
EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA));

Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateB);
EXPECT_FALSE(IsolateIsShutdown(isolateB));
EXPECT_TRUE(IsolateIsRegistered(isolateB));
EXPECT_TRUE(IsolateWasRegistered(isolateB));
EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateB));

Dart_EnterIsolate(isolateA);
Dart_ShutdownIsolate();
EXPECT_TRUE(IsolateIsShutdown(isolateA));
EXPECT_FALSE(IsolateIsRegistered(isolateA));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA));

Dart_EnterIsolate(isolateB);
Dart_ShutdownIsolate();
EXPECT_TRUE(IsolateIsShutdown(isolateB));
EXPECT_FALSE(IsolateIsRegistered(isolateB));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB));

mgr.ShutdownPlatformIsolates();
EXPECT_TRUE(mgr.HasShutdown());

EXPECT_TRUE(IsolateIsShutdown(isolateA));
EXPECT_FALSE(IsolateIsRegistered(isolateA));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA));
EXPECT_TRUE(IsolateIsShutdown(isolateB));
EXPECT_FALSE(IsolateIsRegistered(isolateB));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB));
});
}
Expand All @@ -211,26 +204,24 @@ TEST_F(PlatformIsolateManagerTest, RegistrationAfterShutdown) {
Dart_Isolate isolateA = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateA);
EXPECT_FALSE(IsolateIsShutdown(isolateA));
EXPECT_TRUE(IsolateIsRegistered(isolateA));
EXPECT_TRUE(IsolateWasRegistered(isolateA));
EXPECT_TRUE(mgr.IsRegisteredForTestingOnly(isolateA));

mgr.ShutdownPlatformIsolates();
EXPECT_TRUE(mgr.HasShutdown());

EXPECT_TRUE(IsolateIsShutdown(isolateA));
EXPECT_FALSE(IsolateIsRegistered(isolateA));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateA));

Dart_Isolate isolateB = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolateB);
EXPECT_FALSE(IsolateIsShutdown(isolateB));
EXPECT_FALSE(IsolateIsRegistered(isolateB));
EXPECT_FALSE(IsolateWasRegistered(isolateB));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB));

Dart_EnterIsolate(isolateB);
Dart_ShutdownIsolate();
EXPECT_TRUE(IsolateIsShutdown(isolateB));
EXPECT_FALSE(IsolateIsRegistered(isolateB));
EXPECT_FALSE(mgr.IsRegisteredForTestingOnly(isolateB));
});
}
Expand All @@ -250,9 +241,8 @@ TEST_F(PlatformIsolateManagerTest, MultithreadedCreation) {
for (int j = 0; j < 100; ++j) {
Dart_Isolate isolate = CreateAndRegisterIsolate(&mgr);
ASSERT_TRUE(isolate);
EXPECT_FALSE(IsolateIsShutdown(isolate));

if (!IsolateIsRegistered(isolate)) {
if (!IsolateWasRegistered(isolate)) {
Dart_EnterIsolate(isolate);
Dart_ShutdownIsolate();
}
Expand Down

0 comments on commit ea848c3

Please sign in to comment.