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

Avoid deadlock while creating platform views with multiple engines. #28224

Merged
fluttergithubbot merged 1 commit into from
Aug 23, 2021
Merged

Avoid deadlock while creating platform views with multiple engines. #28224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions fml/raster_thread_merger.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ RasterThreadMerger::RasterThreadMerger(
fml::TaskQueueId gpu_queue_id)
: platform_queue_id_(platform_queue_id),
gpu_queue_id_(gpu_queue_id),
shared_merger_(shared_merger),
enabled_(true) {}
shared_merger_(shared_merger) {}

void RasterThreadMerger::SetMergeUnmergeCallback(const fml::closure& callback) {
merge_unmerge_callback_ = callback;
Expand Down Expand Up @@ -119,12 +118,12 @@ bool RasterThreadMerger::IsMerged() {

void RasterThreadMerger::Enable() {
std::scoped_lock lock(mutex_);
enabled_ = true;
shared_merger_->SetEnabledUnSafe(true);
}

void RasterThreadMerger::Disable() {
std::scoped_lock lock(mutex_);
enabled_ = false;
shared_merger_->SetEnabledUnSafe(false);
}

bool RasterThreadMerger::IsEnabled() {
Expand All @@ -133,7 +132,7 @@ bool RasterThreadMerger::IsEnabled() {
}

bool RasterThreadMerger::IsEnabledUnSafe() const {
return enabled_;
return shared_merger_->IsEnabledUnSafe();
Copy link
Member

@gaaclarke gaaclarke Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit odd that the writing of this is in a mutex lock, where reading is not.

Copy link
Member Author

@eggfly eggfly Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaaclarke Thanks! 💪 I also have some changes about the thread merger, I will deal with it together with another PR.

}

bool RasterThreadMerger::IsMergedUnSafe() const {
Expand Down
1 change: 0 additions & 1 deletion fml/raster_thread_merger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ class RasterThreadMerger
std::condition_variable merged_condition_;
std::mutex mutex_;
fml::closure merge_unmerge_callback_;
bool enabled_;

bool IsMergedUnSafe() const;

Expand Down
21 changes: 21 additions & 0 deletions fml/raster_thread_merger_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,27 @@ TEST(RasterThreadMerger, IsEnabled) {
ASSERT_TRUE(raster_thread_merger->IsEnabled());
}

TEST(RasterThreadMerger, TwoMergersWithSameThreadPairShareEnabledState) {
TaskQueueWrapper queue1;
TaskQueueWrapper queue2;
fml::TaskQueueId qid1 = queue1.GetTaskQueueId();
fml::TaskQueueId qid2 = queue2.GetTaskQueueId();
const auto merger1 =
RasterThreadMerger::CreateOrShareThreadMerger(nullptr, qid1, qid2);
const auto merger2 =
RasterThreadMerger::CreateOrShareThreadMerger(merger1, qid1, qid2);
ASSERT_TRUE(merger1->IsEnabled());
ASSERT_TRUE(merger2->IsEnabled());

merger1->Disable();
ASSERT_FALSE(merger1->IsEnabled());
ASSERT_FALSE(merger2->IsEnabled());

merger2->Enable();
ASSERT_TRUE(merger1->IsEnabled());
ASSERT_TRUE(merger2->IsEnabled());
}

TEST(RasterThreadMerger, RunExpiredTasksWhileFirstTaskMergesThreads) {
fml::MessageLoop* loop_platform = nullptr;
fml::AutoResetWaitableEvent latch1;
Expand Down
11 changes: 10 additions & 1 deletion fml/shared_thread_merger.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ SharedThreadMerger::SharedThreadMerger(fml::TaskQueueId owner,
fml::TaskQueueId subsumed)
: owner_(owner),
subsumed_(subsumed),
task_queues_(fml::MessageLoopTaskQueues::GetInstance()) {}
task_queues_(fml::MessageLoopTaskQueues::GetInstance()),
enabled_(true) {}

bool SharedThreadMerger::MergeWithLease(RasterThreadMergerId caller,
size_t lease_term) {
Expand Down Expand Up @@ -85,6 +86,14 @@ bool SharedThreadMerger::IsMergedUnSafe() const {
return !IsAllLeaseTermsZeroUnSafe();
}

bool SharedThreadMerger::IsEnabledUnSafe() const {
return enabled_;
}

void SharedThreadMerger::SetEnabledUnSafe(bool enabled) {
enabled_ = enabled;
}

Comment on lines +89 to +96
Copy link
Member

@gaaclarke gaaclarke Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit uncertain about which threads are allowed to call this.

Copy link
Member Author

@eggfly eggfly Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will think about it

bool SharedThreadMerger::IsAllLeaseTermsZeroUnSafe() const {
return std::all_of(lease_term_by_caller_.begin(), lease_term_by_caller_.end(),
[&](const auto& item) { return item.second == 0; });
Expand Down
9 changes: 9 additions & 0 deletions fml/shared_thread_merger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ class SharedThreadMerger
// See the doc of |RasterThreadMerger::IsMergedUnSafe|.
bool IsMergedUnSafe() const;

// It's called by |RasterThreadMerger::IsEnabledUnSafe|.
// See the doc of |RasterThreadMerger::IsEnabledUnSafe|.
bool IsEnabledUnSafe() const;

// It's called by |RasterThreadMerger::Enable| or
// |RasterThreadMerger::Disable|.
void SetEnabledUnSafe(bool enabled);

// It's called by |RasterThreadMerger::DecrementLease|.
// See the doc of |RasterThreadMerger::DecrementLease|.
bool DecrementLease(RasterThreadMergerId caller);
Expand All @@ -51,6 +59,7 @@ class SharedThreadMerger
fml::TaskQueueId subsumed_;
fml::RefPtr<fml::MessageLoopTaskQueues> task_queues_;
std::mutex mutex_;
bool enabled_;

/// The |MergeWithLease| or |ExtendLeaseTo| method will record the caller
/// into this lease_term_by_caller_ map, |UnMergeNowIfLastOne|
Expand Down
14 changes: 7 additions & 7 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,17 +709,17 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> surface) {
//
// This prevents false positives such as this method starts assuming that the
// raster and platform queues have a given thread configuration, but then the
// configuration is changed by a task, and the asumption is not longer true.
// configuration is changed by a task, and the assumption is no longer true.
//
// This incorrect assumption can lead to dead lock.
// This incorrect assumption can lead to deadlock.
// See `should_post_raster_task` for more.
rasterizer_->DisableThreadMergerIfNeeded();

// The normal flow executed by this method is that the platform thread is
// starting the sequence and waiting on the latch. Later the UI thread posts
// raster_task to the raster thread which signals the latch. If the raster and
// the platform threads are the same this results in a deadlock as the
// raster_task will never be posted to the plaform/raster thread that is
// raster_task will never be posted to the platform/raster thread that is
// blocked on a latch. To avoid the described deadlock, if the raster and the
// platform threads are the same, should_post_raster_task will be false, and
// then instead of posting a task to the raster thread, the ui thread just
Expand Down Expand Up @@ -818,18 +818,18 @@ void Shell::OnPlatformViewDestroyed() {
//
// This prevents false positives such as this method starts assuming that the
// raster and platform queues have a given thread configuration, but then the
// configuration is changed by a task, and the asumption is not longer true.
// configuration is changed by a task, and the assumption is no longer true.
//
// This incorrect assumption can lead to dead lock.
// This incorrect assumption can lead to deadlock.
// See `should_post_raster_task` for more.
rasterizer_->DisableThreadMergerIfNeeded();

// The normal flow executed by this method is that the platform thread is
// starting the sequence and waiting on the latch. Later the UI thread posts
// raster_task to the raster thread triggers signaling the latch(on the IO
// thread). If the raster and the platform threads are the same this results
// in a deadlock as the raster_task will never be posted to the plaform/raster
// thread that is blocked on a latch. To avoid the described deadlock, if the
// in a deadlock as the raster_task will never be posted to platform/raster
// thread that is blocked on a latch. To avoid the described deadlock, if the
// raster and the platform threads are the same, should_post_raster_task will
// be false, and then instead of posting a task to the raster thread, the ui
// thread just signals the latch and the platform/raster thread follows with
Expand Down