diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.cc b/impeller/renderer/backend/vulkan/resource_manager_vk.cc index 6569f7a61667f..382f7aa715e7c 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk.cc @@ -11,13 +11,15 @@ namespace impeller { std::shared_ptr ResourceManagerVK::Create() { - auto manager = std::shared_ptr(new ResourceManagerVK()); - manager->waiter_ = std::thread([manager]() { manager->Start(); }); - manager->waiter_.detach(); - return manager; + // It will be tempting to refactor this to create the waiter thread in the + // static method instead of the constructor. However, that causes the + // destructor never to be called, and the thread never terminates! + // + // See https://github.com/flutter/flutter/issues/134482. + return std::shared_ptr(new ResourceManagerVK()); } -ResourceManagerVK::ResourceManagerVK() {} +ResourceManagerVK::ResourceManagerVK() : waiter_([&]() { Start(); }) {} ResourceManagerVK::~ResourceManagerVK() { Terminate(); @@ -25,8 +27,10 @@ ResourceManagerVK::~ResourceManagerVK() { } void ResourceManagerVK::Start() { - // This thread should not be started more than once. - FML_DCHECK(!should_exit_); + // It's possible for Start() to be called when terminating: + // { ResourceManagerVK::Create(); } + // + // ... so no FML_DCHECK here. fml::Thread::SetCurrentThreadName( fml::Thread::ThreadConfig{"io.flutter.impeller.resource_manager"}); diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc index dc629b730b1ab..30df654d3dac0 100644 --- a/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/resource_manager_vk_unittests.cc @@ -6,6 +6,7 @@ #include #include #include +#include "fml/logging.h" #include "fml/synchronization/waitable_event.h" #include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/resource_manager_vk.h" @@ -58,5 +59,21 @@ TEST(ResourceManagerVKTest, ReclaimMovesAResourceAndDestroysIt) { waiter.Wait(); } +// Regression test for https://github.com/flutter/flutter/issues/134482. +TEST(ResourceManagerVKTest, TerminatesWhenOutOfScope) { + // Originally, this shared_ptr was never destroyed, and the thread never + // terminated. This test ensures that the thread terminates when the + // ResourceManagerVK is out of scope. + std::weak_ptr manager; + + { + auto shared = ResourceManagerVK::Create(); + manager = shared; + } + + // The thread should have terminated. + EXPECT_EQ(manager.lock(), nullptr); +} + } // namespace testing } // namespace impeller