From dfa94d6bc1290de2009d66e9762e2a21cff4194f Mon Sep 17 00:00:00 2001 From: Alexey Sokolov Date: Sat, 31 Oct 2015 21:08:52 +0000 Subject: [PATCH 1/2] Move Unix signal processing to a dedicated thread. --- include/znc/znc.h | 9 +++-- src/main.cpp | 97 ++++++++++++++++++++++++++++++----------------- 2 files changed, 68 insertions(+), 38 deletions(-) diff --git a/include/znc/znc.h b/include/znc/znc.h index 190b6194..bb9a354c 100644 --- a/include/znc/znc.h +++ b/include/znc/znc.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -100,7 +101,7 @@ public: void AuthUser(std::shared_ptr AuthClass); // Setters - void SetConfigState(enum ConfigState e) { m_eConfigState = e; } + void SetConfigState(enum ConfigState e) { std::lock_guard guard(m_mutexConfigState); m_eConfigState = e; } void SetSkinName(const CString& s) { m_sSkinName = s; } void SetStatusPrefix(const CString& s) { m_sStatusPrefix = (s.empty()) ? "*" : s; } void SetMaxBufferSize(unsigned int i) { m_uiMaxBufferSize = i; } @@ -115,7 +116,7 @@ public: // !Setters // Getters - enum ConfigState GetConfigState() const { return m_eConfigState; } + enum ConfigState GetConfigState() { std::lock_guard guard(m_mutexConfigState); return m_eConfigState; } CSockManager& GetManager() { return m_Manager; } const CSockManager& GetManager() const { return m_Manager; } CModules& GetModules() { return *m_pModules; } @@ -222,7 +223,9 @@ protected: time_t m_TimeStarted; enum ConfigState m_eConfigState; - std::vector m_vpListeners; + std::mutex m_mutexConfigState; + + std::vector m_vpListeners; std::map m_msUsers; std::map m_msDelUsers; CSockManager m_Manager; diff --git a/src/main.cpp b/src/main.cpp index 603411e5..f05b3723 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -22,6 +22,7 @@ #include #include #include +#include static std::vector > lock_cs; @@ -130,24 +131,64 @@ static void GenerateHelp(const char *appname) { CUtils::PrintMessage("\t-d, --datadir Set a different ZNC repository (default is ~/.znc)"); } -static void die(int sig) { - signal(SIGPIPE, SIG_DFL); - CZNC::Get().SetConfigState(CZNC::ECONFIG_NEED_QUIT); -} - -static void signalHandler(int sig) { - switch (sig) { - case SIGHUP: - CZNC::Get().SetConfigState(CZNC::ECONFIG_NEED_REHASH); - break; - case SIGUSR1: - CZNC::Get().SetConfigState(CZNC::ECONFIG_NEED_VERBOSE_WRITE); - break; - default: - // WTF? Signal handler called for a signal it doesn't know? - abort(); +class CSignalHandler { +public: + CSignalHandler(CZNC* pZNC) { + sigset_t signals; + sigfillset(&signals); + pthread_sigmask(SIG_SETMASK, &signals, nullptr); + m_thread = std::thread([=]() { + HandleSignals(pZNC); + }); } -} + ~CSignalHandler() { + pthread_cancel(m_thread.native_handle()); + m_thread.join(); + } +private: + void HandleSignals(CZNC* pZNC) { + sigset_t signals; + sigemptyset(&signals); + sigaddset(&signals, SIGHUP); + sigaddset(&signals, SIGUSR1); + sigaddset(&signals, SIGINT); + sigaddset(&signals, SIGQUIT); + sigaddset(&signals, SIGTERM); + sigaddset(&signals, SIGPIPE); + // Handle only these signals specially; the rest will have their default action, but in this thread + pthread_sigmask(SIG_SETMASK, &signals, nullptr); + while (true) { + int sig; + pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, nullptr); + if (sigwait(&signals, &sig) == -1) continue; + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, nullptr); + switch (sig) { + case SIGHUP: + pZNC->SetConfigState(CZNC::ECONFIG_NEED_REHASH); + break; + case SIGUSR1: + pZNC->SetConfigState(CZNC::ECONFIG_NEED_VERBOSE_WRITE); + break; + case SIGINT: + case SIGQUIT: + case SIGTERM: + pZNC->SetConfigState(CZNC::ECONFIG_NEED_QUIT); + // Reset handler to default by: + // * not blocking it + // * not waiting for it + // So, if ^C is pressed, but for some reason it didn't work, second ^C will kill the process for sure. + sigdelset(&signals, sig); + pthread_sigmask(SIG_SETMASK, &signals, nullptr); + break; + case SIGPIPE: + default: + break; + } + } + } + + std::thread m_thread; +}; static bool isRoot() { // User root? If one of these were root, we could switch the others to root, too @@ -385,24 +426,8 @@ int main(int argc, char** argv) { // controlling terminal). We are independent! } - struct sigaction sa; - sa.sa_flags = 0; - sigemptyset(&sa.sa_mask); - - sa.sa_handler = SIG_IGN; - sigaction(SIGPIPE, &sa, (struct sigaction*) nullptr); - - sa.sa_handler = signalHandler; - sigaction(SIGHUP, &sa, (struct sigaction*) nullptr); - sigaction(SIGUSR1, &sa, (struct sigaction*) nullptr); - - // Once this signal is caught, the signal handler is reset - // to SIG_DFL. This avoids endless loop with signals. - sa.sa_flags = SA_RESETHAND; - sa.sa_handler = die; - sigaction(SIGINT, &sa, (struct sigaction*) nullptr); - sigaction(SIGQUIT, &sa, (struct sigaction*) nullptr); - sigaction(SIGTERM, &sa, (struct sigaction*) nullptr); + // Handle all signals in separate thread + std::unique_ptr SignalHandler(new CSignalHandler(pZNC)); int iRet = 0; @@ -436,6 +461,7 @@ int main(int argc, char** argv) { // The above code adds 3 entries to args tops // which means the array should be big enough + SignalHandler.reset(); CZNC::DestroyInstance(); execvp(args[0], args); CUtils::PrintError("Unable to restart ZNC [" + CString(strerror(errno)) + "]"); @@ -445,6 +471,7 @@ int main(int argc, char** argv) { } } + SignalHandler.reset(); CZNC::DestroyInstance(); CUtils::PrintMessage("Exiting"); From 9c48119897d2ff022515b6646e98d00cebe8c8b2 Mon Sep 17 00:00:00 2001 From: Alexey Sokolov Date: Sat, 31 Oct 2015 22:08:23 +0000 Subject: [PATCH 2/2] Use C++11 threads instead of custom pthread wrappers. Signal handling of CThread was handled in previous commit. --- include/znc/Threads.h | 167 ++---------------------------------------- src/Threads.cpp | 13 ++-- test/ThreadTest.cpp | 6 +- 3 files changed, 16 insertions(+), 170 deletions(-) diff --git a/include/znc/Threads.h b/include/znc/Threads.h index 8590bb25..1bd3a53a 100644 --- a/include/znc/Threads.h +++ b/include/znc/Threads.h @@ -29,179 +29,27 @@ #include #include #include +#include +#include /** * This class represents a non-recursive mutex. Only a single thread may own the * mutex at any point in time. */ -class CMutex { -public: - friend class CConditionVariable; - - CMutex() : m_mutex() { - int i = pthread_mutex_init(&m_mutex, nullptr); - if (i) { - CUtils::PrintError("Can't initialize mutex: " + CString(strerror(errno))); - exit(1); - } - } - - ~CMutex() { - int i = pthread_mutex_destroy(&m_mutex); - if (i) { - CUtils::PrintError("Can't destroy mutex: " + CString(strerror(errno))); - exit(1); - } - } - - void lock() { - int i = pthread_mutex_lock(&m_mutex); - if (i) { - CUtils::PrintError("Can't lock mutex: " + CString(strerror(errno))); - exit(1); - } - } - - void unlock() { - int i = pthread_mutex_unlock(&m_mutex); - if (i) { - CUtils::PrintError("Can't unlock mutex: " + CString(strerror(errno))); - exit(1); - } - } - -private: - // Undefined copy constructor and assignment operator - CMutex(const CMutex&); - CMutex& operator=(const CMutex&); - - pthread_mutex_t m_mutex; -}; +using CMutex = std::mutex; /** * A mutex locker should always be used as an automatic variable. This * class makes sure that the mutex is unlocked when this class is destructed. * For example, this makes it easier to make code exception-safe. */ -class CMutexLocker { -public: - CMutexLocker(CMutex& mutex, bool initiallyLocked = true) - : m_mutex(mutex), m_locked(false) { - if (initiallyLocked) - lock(); - } - - ~CMutexLocker() { - if (m_locked) - unlock(); - } - - void lock() { - assert(!m_locked); - m_mutex.lock(); - m_locked = true; - } - - void unlock() { - assert(m_locked); - m_locked = false; - m_mutex.unlock(); - } - -private: - // Undefined copy constructor and assignment operator - CMutexLocker(const CMutexLocker&); - CMutexLocker& operator=(const CMutexLocker&); - - CMutex &m_mutex; - bool m_locked; -}; +using CMutexLocker = std::unique_lock; /** * A condition variable makes it possible for threads to wait until some * condition is reached at which point the thread can wake up again. */ -class CConditionVariable { -public: - CConditionVariable() : m_cond() { - int i = pthread_cond_init(&m_cond, nullptr); - if (i) { - CUtils::PrintError("Can't initialize condition variable: " - + CString(strerror(errno))); - exit(1); - } - } - - ~CConditionVariable() { - int i = pthread_cond_destroy(&m_cond); - if (i) { - CUtils::PrintError("Can't destroy condition variable: " - + CString(strerror(errno))); - exit(1); - } - } - - void wait(CMutex& mutex) { - int i = pthread_cond_wait(&m_cond, &mutex.m_mutex); - if (i) { - CUtils::PrintError("Can't wait on condition variable: " - + CString(strerror(errno))); - exit(1); - } - } - - void signal() { - int i = pthread_cond_signal(&m_cond); - if (i) { - CUtils::PrintError("Can't signal condition variable: " - + CString(strerror(errno))); - exit(1); - } - } - - void broadcast() { - int i = pthread_cond_broadcast(&m_cond); - if (i) { - CUtils::PrintError("Can't broadcast condition variable: " - + CString(strerror(errno))); - exit(1); - } - } - -private: - // Undefined copy constructor and assignment operator - CConditionVariable(const CConditionVariable&); - CConditionVariable& operator=(const CConditionVariable&); - - pthread_cond_t m_cond; -}; - -class CThread { -public: - typedef void *threadRoutine(void *); - static void startThread(threadRoutine *func, void *arg) { - pthread_t thr; - sigset_t old_sigmask, sigmask; - - /* Block all signals. The thread will inherit our signal mask - * and thus won't ever try to handle signals. - */ - int i = sigfillset(&sigmask); - i |= pthread_sigmask(SIG_SETMASK, &sigmask, &old_sigmask); - i |= pthread_create(&thr, nullptr, func, arg); - i |= pthread_sigmask(SIG_SETMASK, &old_sigmask, nullptr); - i |= pthread_detach(thr); - if (i) { - CUtils::PrintError("Can't start new thread: " - + CString(strerror(errno))); - exit(1); - } - } - -private: - // Undefined constructor - CThread(); -}; +using CConditionVariable = std::condition_variable_any; /** * A job is a task which should run without blocking the main thread. You do @@ -292,11 +140,6 @@ private: void finishJob(CJob *) const; void threadFunc(); - static void *threadPoolFunc(void *arg) { - CThreadPool &pool = *reinterpret_cast(arg); - pool.threadFunc(); - return nullptr; - } // mutex protecting all of these members CMutex m_mutex; diff --git a/src/Threads.cpp b/src/Threads.cpp index 9197d620..1f15f518 100644 --- a/src/Threads.cpp +++ b/src/Threads.cpp @@ -20,6 +20,7 @@ #include #include +#include /* Just an arbitrary limit for the number of idle threads */ static const size_t MAX_IDLE_THREADS = 3; @@ -49,7 +50,7 @@ void CThreadPool::jobDone(CJob* job) { if (oldState == CJob::CANCELLED) { // Signal the main thread that cancellation is done - m_cancellationCond.signal(); + m_cancellationCond.notify_one(); return; } @@ -88,7 +89,7 @@ CThreadPool::~CThreadPool() { m_done = true; while (m_num_threads > 0) { - m_cond.broadcast(); + m_cond.notify_all(); m_exit_cond.wait(m_mutex); } } @@ -133,7 +134,7 @@ void CThreadPool::threadFunc() { m_num_idle--; if (m_num_threads == 0 && m_done) - m_exit_cond.signal(); + m_exit_cond.notify_one(); } void CThreadPool::addJob(CJob *job) { @@ -142,7 +143,7 @@ void CThreadPool::addJob(CJob *job) { // Do we already have a thread which can handle this job? if (m_num_idle > 0) { - m_cond.signal(); + m_cond.notify_one(); return; } @@ -153,7 +154,9 @@ void CThreadPool::addJob(CJob *job) { // Start a new thread for our pool m_num_threads++; - CThread::startThread(threadPoolFunc, this); + std::thread([this]() { + threadFunc(); + }).detach(); } void CThreadPool::cancelJob(CJob *job) { diff --git a/test/ThreadTest.cpp b/test/ThreadTest.cpp index ad9df86c..8f06b14f 100644 --- a/test/ThreadTest.cpp +++ b/test/ThreadTest.cpp @@ -40,14 +40,14 @@ public: // and signal it to exit m_bThreadDone = true; - m_CV.broadcast(); + m_CV.notify_all(); } virtual void runThread() { CMutexLocker locker(m_Mutex); // We are running m_bThreadReady = true; - m_CV.broadcast(); + m_CV.notify_all(); // wait for our exit signal while (!m_bThreadDone) @@ -97,7 +97,7 @@ public: m_Mutex.lock(); // We are running, tell the main thread m_bThreadReady = true; - m_CVThreadReady.broadcast(); + m_CVThreadReady.notify_all(); // Have to unlock here so that wait() can get the mutex m_Mutex.unlock();