From fd944523a1f312b64bc91514f3b168d9706ed304 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Wed, 8 Jul 2020 09:42:31 +0800 Subject: [PATCH 1/5] Lua: Change the TLS callback function type of ThreadLocalState to UpdateCb Signed-off-by: wbpcode --- source/extensions/filters/common/lua/lua.cc | 6 ++++-- source/extensions/filters/common/lua/lua.h | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/common/lua/lua.cc b/source/extensions/filters/common/lua/lua.cc index 3f8b5cd7ec015..4c5c3a0ce01fb 100644 --- a/source/extensions/filters/common/lua/lua.cc +++ b/source/extensions/filters/common/lua/lua.cc @@ -71,8 +71,9 @@ int ThreadLocalState::getGlobalRef(uint64_t slot) { } uint64_t ThreadLocalState::registerGlobal(const std::string& global) { - tls_slot_->runOnAllThreads([this, global]() { - LuaThreadLocal& tls = tls_slot_->getTyped(); + tls_slot_->runOnAllThreads([global](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto { + ASSERT(std::dynamic_pointer_cast(ptr)); + LuaThreadLocal& tls = *std::dynamic_pointer_cast(ptr); lua_getglobal(tls.state_.get(), global.c_str()); if (lua_isfunction(tls.state_.get(), -1)) { tls.global_slots_.push_back(luaL_ref(tls.state_.get(), LUA_REGISTRYINDEX)); @@ -81,6 +82,7 @@ uint64_t ThreadLocalState::registerGlobal(const std::string& global) { lua_pop(tls.state_.get(), 1); tls.global_slots_.push_back(LUA_REFNIL); } + return ptr; }); return current_global_slot_++; diff --git a/source/extensions/filters/common/lua/lua.h b/source/extensions/filters/common/lua/lua.h index b9bb7caa157ea..bc0b8e8652f0d 100644 --- a/source/extensions/filters/common/lua/lua.h +++ b/source/extensions/filters/common/lua/lua.h @@ -386,8 +386,12 @@ class ThreadLocalState : Logger::Loggable { * all threaded workers. */ template void registerType() { - tls_slot_->runOnAllThreads( - [this]() { T::registerType(tls_slot_->getTyped().state_.get()); }); + tls_slot_->runOnAllThreads([](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto { + ASSERT(std::dynamic_pointer_cast(ptr)); + LuaThreadLocal& tls = *std::dynamic_pointer_cast(ptr); + T::registerType(tls.state_.get()); + return ptr; + }); } /** From cd3b07ce720aa891a1f16a13109d71d96a5237a1 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 9 Jul 2020 19:47:32 +0800 Subject: [PATCH 2/5] Lua: add test for thread safety ThreadLocalState Signed-off-by: wbpcode --- test/extensions/filters/common/lua/BUILD | 1 + .../extensions/filters/common/lua/lua_test.cc | 50 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/test/extensions/filters/common/lua/BUILD b/test/extensions/filters/common/lua/BUILD index b6d7bfecd6d54..88d42f01aab0b 100644 --- a/test/extensions/filters/common/lua/BUILD +++ b/test/extensions/filters/common/lua/BUILD @@ -14,6 +14,7 @@ envoy_cc_test( srcs = ["lua_test.cc"], tags = ["skip_on_windows"], deps = [ + "//source/common/thread_local:thread_local_lib", "//source/extensions/filters/common/lua:lua_lib", "//test/mocks:common_lib", "//test/mocks/thread_local:thread_local_mocks", diff --git a/test/extensions/filters/common/lua/lua_test.cc b/test/extensions/filters/common/lua/lua_test.cc index b5770a0b20d79..54ac73c6a15a1 100644 --- a/test/extensions/filters/common/lua/lua_test.cc +++ b/test/extensions/filters/common/lua/lua_test.cc @@ -1,5 +1,6 @@ #include +#include "common/thread_local/thread_local_impl.h" #include "extensions/filters/common/lua/lua.h" #include "test/mocks/common.h" @@ -157,6 +158,55 @@ TEST_F(LuaTest, MarkDead) { lua_gc(cr1->luaState(), LUA_GCCOLLECT, 0); } +class ThreadSafeTest : public testing::Test { +public: + ThreadSafeTest() + : api_(Api::createApiForTest()), main_dispatcher_(api_->allocateDispatcher("main")), + worker_dispatcher_(api_->allocateDispatcher("worker")) {} + + // Use a real dispatcher to verify that callback can be executed correctly. + Api::ApiPtr api_; + Event::DispatcherPtr main_dispatcher_; + Event::DispatcherPtr worker_dispatcher_; + ThreadLocal::InstanceImpl tls_; + + std::unique_ptr state_; +}; + +// Test whether ThreadLocalState can be safely released. +TEST_F(ThreadSafeTest, StateDestructedBeforeWorkerRun) { + const std::string SCRIPT{R"EOF( + function HelloWorld() + print("Hello World!") + end + )EOF"}; + + tls_.registerThread(*main_dispatcher_, true); + EXPECT_EQ(main_dispatcher_.get(), &tls_.dispatcher()); + tls_.registerThread(*worker_dispatcher_, false); + + // Some callback functions waiting to be executed will be added to the dispatcher of the Worker + // thread. The callback functions in the main thread will be executed directly. + state_ = std::make_unique(SCRIPT, tls_); + state_->registerType(); + + main_dispatcher_->run(Event::Dispatcher::RunType::Block); + + // Destroy state_. + state_.reset(nullptr); + + // Start a new worker thread to execute the callback function in the worker dispatcher. + Thread::ThreadPtr thread = Thread::threadFactoryForTest().createThread([this]() { + worker_dispatcher_->run(Event::Dispatcher::RunType::Block); + // Verify we have the expected dispatcher for the new thread thread. + EXPECT_EQ(worker_dispatcher_.get(), &tls_.dispatcher()); + }); + thread->join(); + + tls_.shutdownGlobalThreading(); + tls_.shutdownThread(); +} + } // namespace } // namespace Lua } // namespace Common From e679c264b60eecfe38de2648dad594ed288130e3 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 9 Jul 2020 20:13:44 +0800 Subject: [PATCH 3/5] Lua: correct format error of 'lua_test.cc' Signed-off-by: wbpcode --- test/extensions/filters/common/lua/lua_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/extensions/filters/common/lua/lua_test.cc b/test/extensions/filters/common/lua/lua_test.cc index 54ac73c6a15a1..ddd2f8cf57139 100644 --- a/test/extensions/filters/common/lua/lua_test.cc +++ b/test/extensions/filters/common/lua/lua_test.cc @@ -1,6 +1,7 @@ #include #include "common/thread_local/thread_local_impl.h" + #include "extensions/filters/common/lua/lua.h" #include "test/mocks/common.h" From 7335c7492de55b6c0d2e9a6dd5f724cbcfee7d82 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Thu, 9 Jul 2020 20:29:51 +0800 Subject: [PATCH 4/5] Lua: correct spelling error of new test Signed-off-by: wbpcode --- test/extensions/filters/common/lua/lua_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/common/lua/lua_test.cc b/test/extensions/filters/common/lua/lua_test.cc index ddd2f8cf57139..6d451a8baf54d 100644 --- a/test/extensions/filters/common/lua/lua_test.cc +++ b/test/extensions/filters/common/lua/lua_test.cc @@ -165,7 +165,7 @@ class ThreadSafeTest : public testing::Test { : api_(Api::createApiForTest()), main_dispatcher_(api_->allocateDispatcher("main")), worker_dispatcher_(api_->allocateDispatcher("worker")) {} - // Use a real dispatcher to verify that callback can be executed correctly. + // Use real dispatchers to verify that callback functions can be executed correctly. Api::ApiPtr api_; Event::DispatcherPtr main_dispatcher_; Event::DispatcherPtr worker_dispatcher_; @@ -196,7 +196,7 @@ TEST_F(ThreadSafeTest, StateDestructedBeforeWorkerRun) { // Destroy state_. state_.reset(nullptr); - // Start a new worker thread to execute the callback function in the worker dispatcher. + // Start a new worker thread to execute the callback functions in the worker dispatcher. Thread::ThreadPtr thread = Thread::threadFactoryForTest().createThread([this]() { worker_dispatcher_->run(Event::Dispatcher::RunType::Block); // Verify we have the expected dispatcher for the new thread thread. From 1def9eada89335f7b3b6a0e55ac6b9de5b730814 Mon Sep 17 00:00:00 2001 From: wbpcode Date: Fri, 10 Jul 2020 10:42:31 +0800 Subject: [PATCH 5/5] Lua: update parameter name and some comments Signed-off-by: wbpcode --- source/extensions/filters/common/lua/lua.cc | 7 +++---- source/extensions/filters/common/lua/lua.h | 7 +++---- test/extensions/filters/common/lua/lua_test.cc | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/common/lua/lua.cc b/source/extensions/filters/common/lua/lua.cc index 4c5c3a0ce01fb..e49b0c49703a3 100644 --- a/source/extensions/filters/common/lua/lua.cc +++ b/source/extensions/filters/common/lua/lua.cc @@ -71,9 +71,8 @@ int ThreadLocalState::getGlobalRef(uint64_t slot) { } uint64_t ThreadLocalState::registerGlobal(const std::string& global) { - tls_slot_->runOnAllThreads([global](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto { - ASSERT(std::dynamic_pointer_cast(ptr)); - LuaThreadLocal& tls = *std::dynamic_pointer_cast(ptr); + tls_slot_->runOnAllThreads([global](ThreadLocal::ThreadLocalObjectSharedPtr previous) { + LuaThreadLocal& tls = *std::dynamic_pointer_cast(previous); lua_getglobal(tls.state_.get(), global.c_str()); if (lua_isfunction(tls.state_.get(), -1)) { tls.global_slots_.push_back(luaL_ref(tls.state_.get(), LUA_REGISTRYINDEX)); @@ -82,7 +81,7 @@ uint64_t ThreadLocalState::registerGlobal(const std::string& global) { lua_pop(tls.state_.get(), 1); tls.global_slots_.push_back(LUA_REFNIL); } - return ptr; + return previous; }); return current_global_slot_++; diff --git a/source/extensions/filters/common/lua/lua.h b/source/extensions/filters/common/lua/lua.h index bc0b8e8652f0d..7071b375303fa 100644 --- a/source/extensions/filters/common/lua/lua.h +++ b/source/extensions/filters/common/lua/lua.h @@ -386,11 +386,10 @@ class ThreadLocalState : Logger::Loggable { * all threaded workers. */ template void registerType() { - tls_slot_->runOnAllThreads([](ThreadLocal::ThreadLocalObjectSharedPtr ptr) -> auto { - ASSERT(std::dynamic_pointer_cast(ptr)); - LuaThreadLocal& tls = *std::dynamic_pointer_cast(ptr); + tls_slot_->runOnAllThreads([](ThreadLocal::ThreadLocalObjectSharedPtr previous) { + LuaThreadLocal& tls = *std::dynamic_pointer_cast(previous); T::registerType(tls.state_.get()); - return ptr; + return previous; }); } diff --git a/test/extensions/filters/common/lua/lua_test.cc b/test/extensions/filters/common/lua/lua_test.cc index 6d451a8baf54d..5f4462e7d3c4f 100644 --- a/test/extensions/filters/common/lua/lua_test.cc +++ b/test/extensions/filters/common/lua/lua_test.cc @@ -199,7 +199,7 @@ TEST_F(ThreadSafeTest, StateDestructedBeforeWorkerRun) { // Start a new worker thread to execute the callback functions in the worker dispatcher. Thread::ThreadPtr thread = Thread::threadFactoryForTest().createThread([this]() { worker_dispatcher_->run(Event::Dispatcher::RunType::Block); - // Verify we have the expected dispatcher for the new thread thread. + // Verify we have the expected dispatcher for the new worker thread. EXPECT_EQ(worker_dispatcher_.get(), &tls_.dispatcher()); }); thread->join();