From 9d8cf14560fe0a3e7895c64349086e1c9909278f Mon Sep 17 00:00:00 2001
From: Matan Lurey <matanlurey@users.noreply.github.com>
Date: Mon, 11 Sep 2023 19:14:23 -0700
Subject: [PATCH 1/2] Fix thread leak in ResourceManagerVK.

---
 .../backend/vulkan/resource_manager_vk.cc      | 18 +++++++++++-------
 .../backend/vulkan/resource_manager_vk.h       |  2 +-
 .../vulkan/resource_manager_vk_unittests.cc    | 17 +++++++++++++++++
 3 files changed, 29 insertions(+), 8 deletions(-)

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> ResourceManagerVK::Create() {
-  auto manager = std::shared_ptr<ResourceManagerVK>(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<ResourceManagerVK>(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.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h
index 9ccf34ad95916..1f3bc7e443dd5 100644
--- a/impeller/renderer/backend/vulkan/resource_manager_vk.h
+++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h
@@ -78,7 +78,7 @@ class ResourceManagerVK final
   std::mutex reclaimables_mutex_;
   std::condition_variable reclaimables_cv_;
   Reclaimables reclaimables_;
-  bool should_exit_ = false;
+  bool should_exit_;
   // This should be initialized last since it references the other instance
   // variables.
   std::thread waiter_;
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 <functional>
 #include <memory>
 #include <utility>
+#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<ResourceManagerVK> manager;
+
+  {
+    auto shared = ResourceManagerVK::Create();
+    manager = shared;
+  }
+
+  // The thread should have terminated.
+  EXPECT_EQ(manager.lock(), nullptr);
+}
+
 }  // namespace testing
 }  // namespace impeller

From 874575848e95ef2de06f22805e4c539f545046a2 Mon Sep 17 00:00:00 2001
From: Matan Lurey <matanlurey@users.noreply.github.com>
Date: Mon, 11 Sep 2023 19:29:53 -0700
Subject: [PATCH 2/2] Tweak back.

---
 impeller/renderer/backend/vulkan/resource_manager_vk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/impeller/renderer/backend/vulkan/resource_manager_vk.h b/impeller/renderer/backend/vulkan/resource_manager_vk.h
index 1f3bc7e443dd5..9ccf34ad95916 100644
--- a/impeller/renderer/backend/vulkan/resource_manager_vk.h
+++ b/impeller/renderer/backend/vulkan/resource_manager_vk.h
@@ -78,7 +78,7 @@ class ResourceManagerVK final
   std::mutex reclaimables_mutex_;
   std::condition_variable reclaimables_cv_;
   Reclaimables reclaimables_;
-  bool should_exit_;
+  bool should_exit_ = false;
   // This should be initialized last since it references the other instance
   // variables.
   std::thread waiter_;