From 641d111c23e72d485917e366ae1c4b344687dcb2 Mon Sep 17 00:00:00 2001 From: mihacooper Date: Mon, 11 Sep 2017 15:39:47 +0300 Subject: [PATCH] Fix strong reference race in GC (#65) * Fix strong reference race in GC * Add regress test * Fix type stored in StoredObject * Fix thread input arguments lifetime --- src/cpp/channel.cpp | 1 + src/cpp/garbage-collector.h | 1 + src/cpp/shared-table.cpp | 2 ++ src/cpp/shared-table.h | 1 + src/cpp/stored-object.cpp | 12 +++++++++-- src/cpp/stored-object.h | 1 + src/cpp/threading.cpp | 3 ++- tests/lua/gc-stress.lua | 40 +++++++++++++++++++++++++++++++++++++ tests/lua/tests.lua | 1 + 9 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/lua/gc-stress.lua diff --git a/src/cpp/channel.cpp b/src/cpp/channel.cpp index 19334a6..77565d1 100644 --- a/src/cpp/channel.cpp +++ b/src/cpp/channel.cpp @@ -36,6 +36,7 @@ bool Channel::push(const sol::variadic_args& args) { auto obj = createStoredObject(arg.get()); if (obj->gcHandle()) refs_->insert(obj->gcHandle()); + obj->releaseStrongReference(); array.emplace_back(obj); } if (data_->channel_.empty()) diff --git a/src/cpp/garbage-collector.h b/src/cpp/garbage-collector.h index 268e2b3..35dbe31 100644 --- a/src/cpp/garbage-collector.h +++ b/src/cpp/garbage-collector.h @@ -22,6 +22,7 @@ public: : refs_(new std::set) {} GCObject(const GCObject& init) = default; GCObject(GCObject&& init) = default; + GCObject& operator=(const GCObject& init) = default; virtual ~GCObject() = default; GCObjectHandle handle() const noexcept { return reinterpret_cast(refs_.get()); } diff --git a/src/cpp/shared-table.cpp b/src/cpp/shared-table.cpp index 42eb29d..7de49f0 100644 --- a/src/cpp/shared-table.cpp +++ b/src/cpp/shared-table.cpp @@ -54,6 +54,8 @@ void SharedTable::set(StoredObject&& key, StoredObject&& value) { refs_->insert(key->gcHandle()); if (value->gcHandle()) refs_->insert(value->gcHandle()); + key->releaseStrongReference(); + value->releaseStrongReference(); data_->entries[std::move(key)] = std::move(value); } diff --git a/src/cpp/shared-table.h b/src/cpp/shared-table.h index 51e8767..d4ad95f 100644 --- a/src/cpp/shared-table.h +++ b/src/cpp/shared-table.h @@ -22,6 +22,7 @@ public: SharedTable(); SharedTable(SharedTable&&) = default; SharedTable(const SharedTable& init); + SharedTable& operator=(const SharedTable&) = default; virtual ~SharedTable() = default; static void getUserType(sol::state_view& lua); diff --git a/src/cpp/stored-object.cpp b/src/cpp/stored-object.cpp index 2fa3d2f..46e8026 100644 --- a/src/cpp/stored-object.cpp +++ b/src/cpp/stored-object.cpp @@ -74,12 +74,15 @@ public: template GCObjectHolder(const SolType& luaObject) { assert(luaObject.template is()); - handle_ = luaObject.template as().handle(); + strongRef_ = luaObject.template as(); + handle_ = strongRef_->handle(); assert(GC::instance().has(handle_)); } GCObjectHolder(GCObjectHandle handle) - : handle_(handle) {} + : handle_(handle) { + strongRef_ = GC::instance().get(handle_); + } bool rawCompare(const BaseHolder* other) const final { return handle_ < static_cast*>(other)->handle_; @@ -91,8 +94,13 @@ public: GCObjectHandle gcHandle() const override { return handle_; } + void releaseStrongReference() override { + strongRef_ = sol::nullopt; + } + private: GCObjectHandle handle_; + sol::optional strongRef_; }; // This class is used as a storage for visited sol::tables diff --git a/src/cpp/stored-object.h b/src/cpp/stored-object.h index f4daa84..bedc2ed 100644 --- a/src/cpp/stored-object.h +++ b/src/cpp/stored-object.h @@ -22,6 +22,7 @@ public: virtual const std::type_info& type() { return typeid(*this); } virtual sol::object unpack(sol::this_state state) const = 0; virtual GCObjectHandle gcHandle() const { return GCNull; } + virtual void releaseStrongReference() { } private: BaseHolder(const BaseHolder&) = delete; diff --git a/src/cpp/threading.cpp b/src/cpp/threading.cpp index 4e0fb86..8cdfb31 100644 --- a/src/cpp/threading.cpp +++ b/src/cpp/threading.cpp @@ -171,10 +171,11 @@ void runThread(std::shared_ptr handle, try { { - ScopeGuard reportComplete([=](){ + ScopeGuard reportComplete([handle, &arguments](){ DEBUG << "Finished " << std::endl; // Let's destroy accociated state // to release all resources as soon as possible + arguments.clear(); handle->destroyLua(); }); sol::function userFuncObj = loadString(handle->lua(), strFunction); diff --git a/tests/lua/gc-stress.lua b/tests/lua/gc-stress.lua new file mode 100644 index 0000000..b270f5d --- /dev/null +++ b/tests/lua/gc-stress.lua @@ -0,0 +1,40 @@ +require "bootstrap-tests" + +test.gc_stress.tear_down = default_tear_down + +-- Regress test for simultaneous object creation and removing +-- may cause SIGFAULT, so it's marked as "stress" +test.gc_stress.create_and_collect_in_parallel = function () + function worker() + effil = require "effil" + local nested_table = { + {}, --[[1 level]] + {{}}, --[[2 levels]] + {{{}}}, --[[3 levels]] + {{{{}}}} --[[4 levels]] + } + for i = 1, 100 do + for t = 1, 10 do + local tbl = effil.table(nested_table) + for l = 1, 10 do + tbl[l] = nested_table + end + end + collectgarbage() + effil.gc.collect() + end + end + + local thread_num = 10 + local threads = {} + + for i = 1, thread_num do + threads[i] = effil.thread(worker)(i) + end + + for i = 1, thread_num do + test.equal(threads[i]:wait(), "completed") + end + + test.equal(effil.gc.count(), 1) +end diff --git a/tests/lua/tests.lua b/tests/lua/tests.lua index 400ec62..12c0d1d 100755 --- a/tests/lua/tests.lua +++ b/tests/lua/tests.lua @@ -12,6 +12,7 @@ require "metatable" if os.getenv("STRESS") then require "channel-stress" require "thread-stress" + require "gc-stress" end test.summary() \ No newline at end of file