From a90a6b2a7ba4648b0e597accbab05763660b7d93 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 5 Jul 2018 21:16:50 +0300 Subject: [PATCH 1/6] Make lua data race free Fixes tsan trace report between lua viewscreen and other threads running lua without CoreSuspender lock. But I would assume similar races exists when using lua from console thread, remote thread and vmethods same time. --- depends/lua/CMakeLists.txt | 7 +++++ depends/lualimit.h | 61 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 depends/lualimit.h diff --git a/depends/lua/CMakeLists.txt b/depends/lua/CMakeLists.txt index 7dcae8068..97b62dca6 100644 --- a/depends/lua/CMakeLists.txt +++ b/depends/lua/CMakeLists.txt @@ -95,6 +95,13 @@ LIST(APPEND SRC_LIBLUA ${HDR_LIBLUA}) ADD_LIBRARY ( lua SHARED ${SRC_LIBLUA} ) TARGET_LINK_LIBRARIES ( lua ${LIBS}) +target_include_directories(lua PRIVATE ../) +if (MSVC) + target_compile_options(lua PRIVATE /FI lualimit.h) +else () + target_compile_options(lua PRIVATE -include lualimit.h) +endif () + install(TARGETS lua LIBRARY DESTINATION ${DFHACK_LIBRARY_DESTINATION} RUNTIME DESTINATION ${DFHACK_LIBRARY_DESTINATION}) diff --git a/depends/lualimit.h b/depends/lualimit.h new file mode 100644 index 000000000..009501cce --- /dev/null +++ b/depends/lualimit.h @@ -0,0 +1,61 @@ +/** +Copyright © 2018 Pauli + +This software is provided 'as-is', without any express or implied +warranty. In no event will the authors be held liable for any +damages arising from the use of this software. + +Permission is granted to anyone to use this software for any +purpose, including commercial applications, and to alter it and +redistribute it freely, subject to the following restrictions: + +1. The origin of this software must not be misrepresented; you must + not claim that you wrote the original software. If you use this + software in a product, an acknowledgment in the product + documentation would be appreciated but is not required. + +2. Altered source versions must be plainly marked as such, and + must not be misrepresented as being the original software. + +3. This notice may not be removed or altered from any source + distribution. + */ + +#pragma once + +#ifdef _MSC_VER +#include +#else +#include +#endif + +#include + +/*! \file lualimit.h + * dfhack specific lua porting header that overrides lua defaults for thread + * safety. + */ + +#ifdef _MSC_VER +typedef CRITICAL_SECTION mutex_t; +#else +typedef pthread_mutex_t mutex_t; +#endif + +struct lua_extra_state { + mutex_t* mutex; +}; + +#define luai_mutex(L) ((lua_extra_state*)lua_getextraspace(L))->mutex + +#ifdef _MSC_VER +#define luai_userstateopen(L) luai_mutex(L) = (mutex_t*)malloc(sizeof(mutex_t)); InitializeCriticalSection(luai_mutex(L)) +#define luai_userstateclose(L) lua_unlock(L); DeleteCriticalSection(luai_mutex(L)); free(luai_mutex(L)) +#define lua_lock(L) EnterCriticalSection(luai_mutex(L)) +#define lua_unlock(L) LeaveCriticalSection(luai_mutex(L)) +#else +#define luai_userstateopen(L) luai_mutex(L) = (mutex_t*)malloc(sizeof(mutex_t)); *luai_mutex(L) = PTHREAD_MUTEX_INITIALIZER +#define luai_userstateclose(L) lua_unlock(L); pthread_mutex_destroy(luai_mutex(L)); free(luai_mutex(L)) +#define lua_lock(L) pthread_mutex_lock(luai_mutex(L)) +#define lua_unlock(L) pthread_mutex_unlock(luai_mutex(L)) +#endif From 0ed5c8db39ee51d5dd40f68f9cb68e91aed270c4 Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 5 Jul 2018 21:18:49 +0300 Subject: [PATCH 2/6] Fix data race between threaded init and EventManager The initial run_dfhack_init loads shared state information that is used by EventManager when state changes. There is a small risk that EventManager can handle events while run_dfhack_init is still running. --- library/Core.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/library/Core.cpp b/library/Core.cpp index c406fb6c0..c724a0fcc 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -1427,6 +1427,7 @@ bool Core::loadScriptFile(color_ostream &out, string fname, bool silent) static void run_dfhack_init(color_ostream &out, Core *core) { + CoreSuspender lock; if (!df::global::world || !df::global::ui || !df::global::gview) { out.printerr("Key globals are missing, skipping loading dfhack.init.\n"); From 0605b9601ca94243d4d66f87d8602ec0e1e64994 Mon Sep 17 00:00:00 2001 From: Pauli Date: Sat, 7 Jul 2018 13:13:55 +0300 Subject: [PATCH 3/6] Make Core::started thread safe --- library/Core.cpp | 3 ++- library/include/Core.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index c724a0fcc..222680f62 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -1529,6 +1529,7 @@ Core::Core() : HotkeyMutex{}, HotkeyCond{}, alias_mutex{}, + started{false}, misc_data_mutex{}, CoreSuspendMutex{}, CoreWakeup{}, @@ -1538,7 +1539,7 @@ Core::Core() : // init the console. This must be always the first step! plug_mgr = 0; errorstate = false; - started = false; + vinfo = 0; memset(&(s_mods), 0, sizeof(s_mods)); // set up hotkey capture diff --git a/library/include/Core.h b/library/include/Core.h index 0fec2774d..7c3bc5578 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -283,7 +283,7 @@ namespace DFHack df::viewscreen *top_viewscreen; bool last_pause_state; // Very important! - bool started; + std::atomic started; // Additional state change scripts std::vector state_change_scripts; From 49f3de979f41a20ab1a84d04953f2042e0b1e677 Mon Sep 17 00:00:00 2001 From: Pauli Date: Sat, 7 Jul 2018 13:13:55 +0300 Subject: [PATCH 4/6] Make ServerMain and ServerConnection data race free RemoteServer and PluginManager side would need complete redesign to be data race free and concurrent. But as that would be unlikely to be required from DFHack I decided simpler solution that is fixing data ownership to a thread and all ServerConnection share a single lock which allows access to PluginManager and Core. --- library/Core.cpp | 9 +-- library/RemoteServer.cpp | 127 +++++++++++++++++++++------------ library/include/Core.h | 1 - library/include/RemoteServer.h | 21 +++--- 4 files changed, 98 insertions(+), 60 deletions(-) diff --git a/library/Core.cpp b/library/Core.cpp index 222680f62..15d4be72c 100644 --- a/library/Core.cpp +++ b/library/Core.cpp @@ -1549,7 +1549,6 @@ Core::Core() : last_pause_state = false; top_viewscreen = NULL; screen_window = NULL; - server = NULL; color_ostream::log_errors_to_stderr = true; @@ -1767,6 +1766,8 @@ bool Core::Init() // create plugin manager plug_mgr = new PluginManager(this); plug_mgr->init(); + cerr << "Starting the TCP listener.\n"; + auto listen = ServerMain::listen(RemoteClient::GetDefaultPort()); IODATA *temp = new IODATA; temp->core = this; temp->plug_mgr = plug_mgr; @@ -1791,9 +1792,7 @@ bool Core::Init() started = true; modstate = 0; - cerr << "Starting the TCP listener.\n"; - server = new ServerMain(); - if (!server->listen(RemoteClient::GetDefaultPort())) + if (!listen.get()) cerr << "TCP listen failed.\n"; if (df::global::ui_sidebar_menus) @@ -2296,6 +2295,8 @@ int Core::Shutdown ( void ) HotkeyCond.notify_one(); } + ServerMain::block(); + d->hotkeythread.join(); d->iothread.join(); diff --git a/library/RemoteServer.cpp b/library/RemoteServer.cpp index 86ef9f40c..523058d55 100644 --- a/library/RemoteServer.cpp +++ b/library/RemoteServer.cpp @@ -57,12 +57,11 @@ POSSIBILITY OF SUCH DAMAGE. #include #include +#include #include "json/json.h" -#include "tinythread.h" using namespace DFHack; -using namespace tthread; using dfproto::CoreTextNotification; using dfproto::CoreTextFragment; @@ -72,6 +71,30 @@ bool readFullBuffer(CSimpleSocket *socket, void *buf, int size); bool sendRemoteMessage(CSimpleSocket *socket, int16_t id, const ::google::protobuf::MessageLite *msg, bool size_ready); +std::mutex ServerMain::access_{}; +bool ServerMain::blocked_{}; + +namespace { + struct BlockedException : std::exception { + const char* what() const noexcept override + { + return "Core has blocked all connection. This should have been catched."; + } + }; +} + +namespace DFHack { + + struct BlockGuard { + std::lock_guard lock; + BlockGuard() : + lock{ServerMain::access_} + { + if (ServerMain::blocked_) + throw BlockedException{}; + } + }; +} RPCService::RPCService() { @@ -134,9 +157,6 @@ ServerConnection::ServerConnection(CActiveSocket *socket) core_service = new CoreService(); core_service->finalize(this, &functions); - - thread = new tthread::thread(threadFn, (void*)this); - thread->detach(); } ServerConnection::~ServerConnection() @@ -144,7 +164,6 @@ ServerConnection::~ServerConnection() in_error = true; socket->Close(); delete socket; - delete thread; for (auto it = plugin_services.begin(); it != plugin_services.end(); ++it) delete it->second; @@ -216,13 +235,14 @@ void ServerConnection::connection_ostream::flush_proxy() } } -void ServerConnection::threadFn(void *arg) +void ServerConnection::Accepted(CActiveSocket* socket) { - ServerConnection *me = (ServerConnection*)arg; - - me->threadFn(); - - delete me; + std::thread{[](CActiveSocket* socket) { + try { + ServerConnection(socket).threadFn(); + } catch (BlockedException e) { + } + }, socket}.detach(); } void ServerConnection::threadFn() @@ -292,6 +312,7 @@ void ServerConnection::threadFn() // Find and call the function int in_size = header.size; + BlockGuard lock; ServerFunctionBase *fn = vector_get(functions, header.id); MessageLite *reply = NULL; @@ -378,25 +399,21 @@ void ServerConnection::threadFn() std::cerr << "Shutting down client connection." << endl; } -ServerMain::ServerMain() -{ - socket = new CPassiveSocket(); - thread = NULL; -} +namespace { + + struct ServerMainImpl : public ServerMain { + CPassiveSocket socket; + static void threadFn(std::promise promise, int port); + ServerMainImpl(std::promise promise, int port); + ~ServerMainImpl(); + }; -ServerMain::~ServerMain() -{ - socket->Close(); - delete socket; - delete thread; } -bool ServerMain::listen(int port) +ServerMainImpl::ServerMainImpl(std::promise promise, int port) : + socket{} { - if (thread) - return true; - - socket->Initialize(); + socket.Initialize(); std::string filename("dfhack-config/remote-server.json"); @@ -427,29 +444,49 @@ bool ServerMain::listen(int port) } std::cerr << "Listening on port " << port << (allow_remote ? " (remote enabled)" : "") << std::endl; - if (allow_remote) - { - if (!socket->Listen(NULL, port)) - return false; - } - else - { - if (!socket->Listen("127.0.0.1", port)) - return false; + const char* addr = allow_remote ? NULL : "127.0.0.1"; + if (!socket.Listen(addr, port)) { + promise.set_value(false); + return; } + promise.set_value(true); +} + +ServerMainImpl::~ServerMainImpl() +{ + socket.Close(); +} - thread = new tthread::thread(threadFn, this); - thread->detach(); - return true; +std::future ServerMain::listen(int port) +{ + std::promise promise; + auto rv = promise.get_future(); + std::thread{&ServerMainImpl::threadFn, std::move(promise), port}.detach(); + return rv; } -void ServerMain::threadFn(void *arg) +void ServerMainImpl::threadFn(std::promise promise, int port) { - ServerMain *me = (ServerMain*)arg; - CActiveSocket *client; + ServerMainImpl server{std::move(promise), port}; - while ((client = me->socket->Accept()) != NULL) - { - new ServerConnection(client); + CActiveSocket *client = nullptr; + + try { + while ((client = server.socket.Accept()) != NULL) + { + BlockGuard lock; + ServerConnection::Accepted(client); + client = nullptr; + } + } catch(BlockedException e) { + if (client) + client->Close(); + delete client; } } + +void ServerMain::block() +{ + std::lock_guard lock{access_}; + blocked_ = true; +} diff --git a/library/include/Core.h b/library/include/Core.h index 7c3bc5578..85bfdbd3e 100644 --- a/library/include/Core.h +++ b/library/include/Core.h @@ -308,7 +308,6 @@ namespace DFHack friend class CoreSuspenderBase; friend struct CoreSuspendClaimMain; friend struct CoreSuspendReleaseMain; - ServerMain *server; }; class CoreSuspenderBase : protected std::unique_lock { diff --git a/library/include/RemoteServer.h b/library/include/RemoteServer.h index 85ac463fb..a7e0ad8d5 100644 --- a/library/include/RemoteServer.h +++ b/library/include/RemoteServer.h @@ -28,6 +28,8 @@ distribution. #include "RemoteClient.h" #include "Core.h" +#include + class CPassiveSocket; class CActiveSocket; class CSimpleSocket; @@ -233,26 +235,25 @@ namespace DFHack CoreService *core_service; std::map plugin_services; - tthread::thread *thread; - static void threadFn(void *); void threadFn(); + ServerConnection(CActiveSocket* socket); + ~ServerConnection(); public: - ServerConnection(CActiveSocket *socket); - ~ServerConnection(); + + static void Accepted(CActiveSocket* socket); ServerFunctionBase *findFunction(color_ostream &out, const std::string &plugin, const std::string &name); }; class ServerMain { - CPassiveSocket *socket; + static std::mutex access_; + static bool blocked_; + friend struct BlockGuard; - tthread::thread *thread; - static void threadFn(void *); public: - ServerMain(); - ~ServerMain(); - bool listen(int port); + static std::future listen(int port); + static void block(); }; } From ea0105fa66ba53113756b5f193fbf2613eb8ee31 Mon Sep 17 00:00:00 2001 From: lethosor Date: Sat, 20 Jul 2019 11:21:44 -0400 Subject: [PATCH 5/6] Add CMake option to provide custom libstdc++ on macOS (#1344) --- CMakeLists.txt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 575f40288..9a9a03caf 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -296,6 +296,7 @@ if(WIN32) endif() endif() +option(EXTERNAL_LIBSTDCXX "macOS only: Avoid installing the DFHack-provided libstdc++." OFF) if(APPLE) # libstdc++ (GCC 4.8.5 for OS X 10.6) # fixes crash-on-unwind bug in DF's libstdc++ @@ -346,9 +347,10 @@ if(APPLE) endif() endif() - install(PROGRAMS ${LIBSTDCXX_DOWNLOAD_DIR}/libstdc++.6.dylib - DESTINATION ./hack/) - + if(NOT EXTERNAL_LIBSTDCXX) + install(PROGRAMS ${LIBSTDCXX_DOWNLOAD_DIR}/libstdc++.6.dylib + DESTINATION ./hack/) + endif() endif() #### expose depends #### From b6678d72aec7fa7d5d39d3c21083c6b05b2673ee Mon Sep 17 00:00:00 2001 From: lethosor Date: Sat, 20 Jul 2019 14:26:30 -0400 Subject: [PATCH 6/6] Move custom lualimit.h to lua dir and rename to dfhack_llimits.h --- depends/lua/CMakeLists.txt | 5 ++--- depends/{lualimit.h => lua/include/dfhack_llimits.h} | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) rename depends/{lualimit.h => lua/include/dfhack_llimits.h} (98%) diff --git a/depends/lua/CMakeLists.txt b/depends/lua/CMakeLists.txt index 97b62dca6..fd3a59c08 100644 --- a/depends/lua/CMakeLists.txt +++ b/depends/lua/CMakeLists.txt @@ -95,11 +95,10 @@ LIST(APPEND SRC_LIBLUA ${HDR_LIBLUA}) ADD_LIBRARY ( lua SHARED ${SRC_LIBLUA} ) TARGET_LINK_LIBRARIES ( lua ${LIBS}) -target_include_directories(lua PRIVATE ../) if (MSVC) - target_compile_options(lua PRIVATE /FI lualimit.h) + target_compile_options(lua PRIVATE /FI dfhack_llimits.h) else () - target_compile_options(lua PRIVATE -include lualimit.h) + target_compile_options(lua PRIVATE -include dfhack_llimits.h) endif () install(TARGETS lua diff --git a/depends/lualimit.h b/depends/lua/include/dfhack_llimits.h similarity index 98% rename from depends/lualimit.h rename to depends/lua/include/dfhack_llimits.h index 009501cce..f8ca51819 100644 --- a/depends/lualimit.h +++ b/depends/lua/include/dfhack_llimits.h @@ -31,7 +31,7 @@ redistribute it freely, subject to the following restrictions: #include -/*! \file lualimit.h +/*! \file dfhack_llimits.h * dfhack specific lua porting header that overrides lua defaults for thread * safety. */