Fix raise condition in GC objects construction (#154)

* Fix raise condition in GC objects construction

* review fixes

src/cpp/channel.cpp

@@ -14,11 +14,13 @@ void Channel::exportAPI(sol::state_view& lua) {
     sol::stack::pop<sol::object>(lua);
 }
 
-Channel::Channel(const sol::stack_object& capacity) {
+void Channel::initialize(const sol::stack_object& capacity) {
     if (capacity.valid()) {
-        REQUIRE(capacity.get_type() == sol::type::number) << "bad argument #1 to 'effil.channel' (number expected, got "
-                                                          << luaTypename(capacity) << ")";
-        REQUIRE(capacity.as<int>() >= 0) << "effil.channel: invalid capacity value = " << capacity.as<int>();
+        REQUIRE(capacity.get_type() == sol::type::number)
+                << "bad argument #1 to 'effil.channel' (number expected, got "
+                << luaTypename(capacity) << ")";
+        REQUIRE(capacity.as<int>() >= 0)
+                << "effil.channel: invalid capacity value = " << capacity.as<int>();
         ctx_->capacity_ = capacity.as<size_t>();
     }
     else {

src/cpp/channel.h

@@ -27,11 +27,9 @@ public:
 
     size_t size();
 
-public:
-    Channel() = delete;
-
 private:
-    explicit Channel(const sol::stack_object& capacity);
+    Channel() = default;
+    void initialize(const sol::stack_object& capacity);
     friend class GC;
 };
 

src/cpp/function.cpp

@@ -2,16 +2,12 @@
 
 namespace effil {
 
-Function::Function(const sol::function& luaObject) {
+void Function::initialize(const sol::function& luaObject) {
     SolTableToShared visited;
-    construct(luaObject, visited);
+    initialize(luaObject, visited);
 }
 
-Function::Function(const sol::function& luaObject, SolTableToShared& visited) {
-    construct(luaObject, visited);
-}
-
-void Function::construct(const sol::function& luaObject, SolTableToShared& visited) {
+void Function::initialize(const sol::function& luaObject, SolTableToShared& visited) {
     assert(luaObject.valid());
     assert(luaObject.get_type() == sol::type::function);
 

src/cpp/function.h

@@ -24,10 +24,10 @@ public:
 private:
     using Converter = std::function<sol::object(const StoredObject&)>;
     sol::object convert(lua_State* state, const Converter& clbk) const;
-    void construct(const sol::function& luaObject, SolTableToShared& visited);
 
-    explicit Function(const sol::function& luaObject, SolTableToShared& visited);
-    explicit Function(const sol::function& luaObject);
+    Function() = default;
+    void initialize(const sol::function& luaObject, SolTableToShared& visited);
+    void initialize(const sol::function& luaObject);
     friend class GC;
 };
 

src/cpp/garbage-collector.h

@@ -20,11 +20,15 @@ public:
         if (enabled_ && count() >= step_ * lastCleanup_)
             collect();
 
-        std::unique_ptr<ViewType> object(new ViewType(std::forward<Args>(args)...));
+        std::unique_lock<std::mutex> g(lock_);
+        std::unique_ptr<ViewType> object(new ViewType);
         auto copy = *object;
-
-        std::lock_guard<std::mutex> g(lock_);
         objects_.emplace(object->handle(), std::move(object));
+        g.unlock();
+
+        // We separate initialization out of construction cause the object under construction
+        // should be added into GC before it will try to put any other objects into it's references
+        copy.initialize(std::forward<Args>(args)...);
         return copy;
     }
 

src/cpp/shared-table.h

@@ -77,6 +77,7 @@ private:
 
 private:
     SharedTable() = default;
+    void initialize() {}
     friend class GC;
 };
 

src/cpp/threading.cpp

@@ -179,12 +179,13 @@ void sleep(const sol::stack_object& duration, const sol::stack_object& metric) {
     }
 }
 
-Thread::Thread(const std::string& path,
-       const std::string& cpath,
-       int step,
-       const sol::function& function,
-       const sol::variadic_args& variadicArgs) {
-
+void Thread::initialize(
+    const std::string& path,
+    const std::string& cpath,
+    int step,
+    const sol::function& function,
+    const sol::variadic_args& variadicArgs)
+{
     sol::optional<Function> functionObj;
     try {
         functionObj = GC::instance().create<Function>(function);

src/cpp/threading.h

@@ -106,11 +106,13 @@ public:
     void resume();
 
 private:
-    Thread(const std::string& path,
-           const std::string& cpath,
-           int step,
-           const sol::function& function,
-           const sol::variadic_args& args);
+    Thread() = default;
+    void initialize(
+        const std::string& path,
+        const std::string& cpath,
+        int step,
+        const sol::function& function,
+        const sol::variadic_args& args);
     friend class GC;
 
 private:

tests/lua/gc-stress.lua

@@ -35,3 +35,21 @@ test.gc_stress.create_and_collect_in_parallel = function ()
         test.equal(threads[i]:wait(), "completed")
     end
 end
+
+test.gc_stress.regress_for_concurent_thread_creation = function ()
+    local a = function() end
+    local b = function() end
+
+    for i = 1, 2000 do
+        effil.thread(function(a, b) a() b() end)(a, b)
+    end
+end
+
+test.gc_stress.regress_for_concurent_function_creation = function ()
+    local a = function() end
+    local b = function() end
+
+    for i = 1, 2000 do
+        effil.thread(function() a() b() end)()
+    end
+end