diff options
-rw-r--r-- | platform/default/thread_local.cpp | 6 | ||||
-rw-r--r-- | platform/qt/src/thread_local.cpp | 7 | ||||
-rw-r--r-- | scripts/valgrind.sup | 8 | ||||
-rw-r--r-- | test/util/thread_local.test.cpp | 28 |
4 files changed, 29 insertions, 20 deletions
diff --git a/platform/default/thread_local.cpp b/platform/default/thread_local.cpp index b754a04b7d..6fdb1e6dc1 100644 --- a/platform/default/thread_local.cpp +++ b/platform/default/thread_local.cpp @@ -29,7 +29,11 @@ ThreadLocal<T>::ThreadLocal() : impl(std::make_unique<Impl>()) { template <class T> ThreadLocal<T>::~ThreadLocal() { - delete get(); + // ThreadLocal will not take ownership + // of the pointer it is managing. The pointer + // needs to be explicitly cleared before we + // destroy this object. + assert(!get()); if (pthread_key_delete(impl->key)) { Log::Error(Event::General, "Failed to delete local storage key."); diff --git a/platform/qt/src/thread_local.cpp b/platform/qt/src/thread_local.cpp index e48a9d6e74..e835a680e2 100644 --- a/platform/qt/src/thread_local.cpp +++ b/platform/qt/src/thread_local.cpp @@ -4,6 +4,7 @@ #include <mbgl/map/backend_scope.hpp> #include <array> +#include <cassert> #include <QThreadStorage> @@ -23,7 +24,11 @@ ThreadLocal<T>::ThreadLocal() : impl(std::make_unique<Impl>()) { template <class T> ThreadLocal<T>::~ThreadLocal() { - delete get(); + // ThreadLocal will not take ownership + // of the pointer it is managing. The pointer + // needs to be explicitly cleared before we + // destroy this object. + assert(!get()); } template <class T> diff --git a/scripts/valgrind.sup b/scripts/valgrind.sup index f659122dc1..09e2f6685c 100644 --- a/scripts/valgrind.sup +++ b/scripts/valgrind.sup @@ -76,11 +76,3 @@ fun:_ZN4mbgl4util10write_fileERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES8_ ... } -{ - Qt5 ThreadStorage - Memcheck:Leak - match-leak-kinds: definite - fun:_Znwm - fun:_Z24qThreadStorage_localDataISt5arrayIPiLm1EEERT_R18QThreadStorageDataPS3_.isra.5 - ... -} diff --git a/test/util/thread_local.test.cpp b/test/util/thread_local.test.cpp index 12a19ab59b..7142697f48 100644 --- a/test/util/thread_local.test.cpp +++ b/test/util/thread_local.test.cpp @@ -71,30 +71,38 @@ TEST(ThreadLocalStorage, NotSetReturnsNull) { namespace { -class TestThreadReclaim { +class TestThreadDataOwnership { public: - TestThreadReclaim(mbgl::ActorRef<TestThreadReclaim>, int* data_) { + TestThreadDataOwnership(mbgl::ActorRef<TestThreadDataOwnership>, int* data_) { data.set(data_); } + ~TestThreadDataOwnership() { + data.set(nullptr); + } + private: - ThreadLocal<int> data; + static ThreadLocal<int> data; }; +ThreadLocal<int> TestThreadDataOwnership::data; + } // namespace -TEST(ThreadLocalStorage, AutoReclaim) { +TEST(ThreadLocalStorage, ShouldNotTakeOwnership) { RunLoop loop; - auto data1 = new int; - auto data2 = new int; + auto data1 = std::make_unique<int>(10); + auto data2 = std::make_unique<int>(20); - auto thread1 = std::make_unique<Thread<TestThreadReclaim>>("Test", data1); - auto thread2 = std::make_unique<Thread<TestThreadReclaim>>("Test", data2); + auto thread1 = std::make_unique<Thread<TestThreadDataOwnership>>("Test", data1.get()); + auto thread2 = std::make_unique<Thread<TestThreadDataOwnership>>("Test", data2.get()); thread1.reset(); thread2.reset(); - // Should not leak, valgrind will - // let us know. + // Will crash if ThreadLocal destroys + // the pointer it is managing. + ASSERT_EQ(*data1, 10); + ASSERT_EQ(*data2, 20); } |