From 94c7b042458052fbb2057c049597d9141e1f91f5 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Wed, 25 Jan 2012 21:15:04 +0100 Subject: [PATCH 1/5] Threaded DNS: Remove TDNS mutex POSIX actually guarantees for us that this white is atomic and thus a partial write is not allowed. Signed-off-by: Uli Schlachter --- include/znc/Socket.h | 2 -- src/Socket.cpp | 31 ++++++------------------------- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/include/znc/Socket.h b/include/znc/Socket.h index e64ac7ff..037d202b 100644 --- a/include/znc/Socket.h +++ b/include/znc/Socket.h @@ -103,7 +103,6 @@ private: #ifdef HAVE_THREADED_DNS int m_iTDNSpipe[2]; - pthread_mutex_t* m_mTDNSmutex; class CTDNSMonitorFD; friend class CTDNSMonitorFD; @@ -124,7 +123,6 @@ private: struct TDNSArg { CString sHostname; TDNSTask* task; - pthread_mutex_t* mutex; int fd; bool bBind; diff --git a/src/Socket.cpp b/src/Socket.cpp index 2b102dca..a1849734 100644 --- a/src/Socket.cpp +++ b/src/Socket.cpp @@ -76,21 +76,15 @@ void* CSockManager::TDNSThread(void* argument) { sleep(5); // wait 5 seconds before next try } - pthread_mutex_t* mutex = a->mutex; - pthread_mutex_lock(mutex); - int wrote = 0; int need = sizeof(TDNSArg*); char* x = (char*)&a; - while (wrote < need) { - int w = write(a->fd, x, need - wrote); - if (-1 == w) { - DEBUG("Something bad happened during write() to a pipe from TDNSThread: " << strerror(errno)); - exit(1); - } - wrote += w; - x += w; + // This write() must succeed because POSIX guarantees that writes of + // less than PIPE_BUF are atomic (and PIPE_BUF is at least 512). + int w = write(a->fd, x, need); + if (w != need) { + DEBUG("Something bad happened during write() to a pipe from TDNSThread, wrote " << w << " bytes: " << strerror(errno)); + exit(1); } - pthread_mutex_unlock(mutex); return NULL; } @@ -99,7 +93,6 @@ void CSockManager::StartTDNSThread(TDNSTask* task, bool bBind) { TDNSArg* arg = new TDNSArg; arg->sHostname = sHostname; arg->task = task; - arg->mutex = m_mTDNSmutex; arg->fd = m_iTDNSpipe[1]; arg->bBind = bBind; arg->iRes = 0; @@ -237,12 +230,6 @@ void CSockManager::RetrieveTDNSResult() { CSockManager::CSockManager() { #ifdef HAVE_THREADED_DNS - m_mTDNSmutex = new pthread_mutex_t; - int m = pthread_mutex_init(m_mTDNSmutex, NULL); - if (m) { - DEBUG("Ouch, can't init mutex for threaded DNS resolving: " << strerror(m)); - exit(1); - } if (pipe(m_iTDNSpipe)) { DEBUG("Ouch, can't open pipe for threaded DNS resolving: " << strerror(errno)); exit(1); @@ -253,12 +240,6 @@ CSockManager::CSockManager() { } CSockManager::~CSockManager() { -#ifdef HAVE_THREADED_DNS - // Here pthread_mutex_destroy(m_mTDNSmutex); and delete m_mTDNSmutex; should be called... - // But lifetime of CSockManager is the same of CZNC, and if to destroy the mutex now, - // lock() from inside TDNSThread will fail. - // So here is a resource and memory leak, but this znc process will die in few moments anyway. -#endif } void CSockManager::Connect(const CString& sHostname, u_short iPort, const CString& sSockName, int iTimeout, bool bSSL, const CString& sBindHost, CZNCSock *pcSock) { From 233897b3a8d16097ca9064e0ff74a9836a1ffc93 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Wed, 25 Jan 2012 21:18:57 +0100 Subject: [PATCH 2/5] threaded DNS: Remove an unneeded function This inlines the function into its only caller. Signed-off-by: Uli Schlachter --- include/znc/Socket.h | 1 - src/Socket.cpp | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/include/znc/Socket.h b/include/znc/Socket.h index 037d202b..50ccccee 100644 --- a/include/znc/Socket.h +++ b/include/znc/Socket.h @@ -131,7 +131,6 @@ private: }; void StartTDNSThread(TDNSTask* task, bool bBind); void SetTDNSThreadFinished(TDNSTask* task, bool bBind, addrinfo* aiResult); - void TryToFinishTDNSTask(TDNSTask* task); void RetrieveTDNSResult(); static void* TDNSThread(void* argument); #endif diff --git a/src/Socket.cpp b/src/Socket.cpp index a1849734..58e3dddf 100644 --- a/src/Socket.cpp +++ b/src/Socket.cpp @@ -129,13 +129,13 @@ void CSockManager::SetTDNSThreadFinished(TDNSTask* task, bool bBind, addrinfo* a task->aiTarget = aiResult; task->bDoneTarget = true; } - TryToFinishTDNSTask(task); -} -void CSockManager::TryToFinishTDNSTask(TDNSTask* task) { + // Now that something is done, check if everything we needed is done if (!task->bDoneBind || !task->bDoneTarget) { return; } + + // All needed DNS is done, now collect the results addrinfo* aiTarget = NULL; addrinfo* aiBind = NULL; From 1d16d1ce7d43034af25bdcf07f7a9631f14cbcbc Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 4 Feb 2012 14:14:16 +0100 Subject: [PATCH 3/5] Fix a stupid bug in CUser::~CUser() A CIRCNetwork's destructor removes the network from its associated user's list of networks. Now that you know this, stare at the diff until you figure out the problem. Yeah, "ouch". Signed-off-by: Uli Schlachter --- src/User.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/User.cpp b/src/User.cpp index cdf1c8b0..45c75828 100644 --- a/src/User.cpp +++ b/src/User.cpp @@ -89,11 +89,9 @@ CUser::CUser(const CString& sUserName) CUser::~CUser() { // Delete networks - for (unsigned int c = 0; c < m_vIRCNetworks.size(); c++) { - CIRCNetwork* pNetwork = m_vIRCNetworks[c]; - delete pNetwork; + while (!m_vIRCNetworks.empty()) { + delete *m_vIRCNetworks.begin(); } - m_vIRCNetworks.clear(); // Delete clients for (unsigned int c = 0; c < m_vClients.size(); c++) { From 4d1e97ed4775726c2e5e3207f3a9c866dc5aefe5 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 4 Feb 2012 16:08:45 +0100 Subject: [PATCH 4/5] Fix AX_PTHREAD to actually work At first the test was failing with the following error due to my CXXFLAGS: conftest.cpp:20:37: error: parameter 'a' set but not used [-Werror=unused-but-set-parameter] Then it figured out that -lpthread works, but I wondered why the test for "-pthread" still fails. This is where I realized that it is testing with C++ compiler, but puts its stuff in $CFLAGS which is just ignored. So a little search-and-replace later, this now uses $CXXFLAGS. At this point the results of the tests looked good, but znc failed to link, because it didn't use -pthread when linking. One little "PTHREAD_LIBS=" later, that problem was also gone and I can finally commit this. Yay. Signed-off-by: Uli Schlachter --- m4/ax_pthread.m4 | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/m4/ax_pthread.m4 b/m4/ax_pthread.m4 index 60239852..2a0447af 100644 --- a/m4/ax_pthread.m4 +++ b/m4/ax_pthread.m4 @@ -98,11 +98,11 @@ ax_pthread_ok=no # etcetera environment variables, and if threads linking works using # them: if test x"$PTHREAD_LIBS$PTHREAD_CFLAGS" != x; then - save_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS $PTHREAD_CFLAGS" + save_CXXFLAGS="$CXXFLAGS" + CXXFLAGS="$CXXFLAGS $PTHREAD_CFLAGS" save_LIBS="$LIBS" LIBS="$PTHREAD_LIBS $LIBS" - AC_MSG_CHECKING([for pthread_join in LIBS=$PTHREAD_LIBS with CFLAGS=$PTHREAD_CFLAGS]) + AC_MSG_CHECKING([for pthread_join in LIBS=$PTHREAD_LIBS with CXXFLAGS=$PTHREAD_CFLAGS]) AC_TRY_LINK_FUNC(pthread_join, ax_pthread_ok=yes) AC_MSG_RESULT($ax_pthread_ok) if test x"$ax_pthread_ok" = xno; then @@ -110,7 +110,7 @@ if test x"$PTHREAD_LIBS$PTHREAD_CFLAGS" != x; then PTHREAD_CFLAGS="" fi LIBS="$save_LIBS" - CFLAGS="$save_CFLAGS" + CXXFLAGS="$save_CXXFLAGS" fi # We must check for the threads library under a number of different @@ -175,6 +175,7 @@ for flag in $ax_pthread_flags; do -*) AC_MSG_CHECKING([whether pthreads work with $flag]) PTHREAD_CFLAGS="$flag" + PTHREAD_LIBS="$flag" ;; pthread-config) @@ -191,9 +192,9 @@ for flag in $ax_pthread_flags; do esac save_LIBS="$LIBS" - save_CFLAGS="$CFLAGS" + save_CXXFLAGS="$CXXFLAGS" LIBS="$PTHREAD_LIBS $LIBS" - CFLAGS="$CFLAGS $PTHREAD_CFLAGS" + CXXFLAGS="$CXXFLAGS $PTHREAD_CFLAGS" # Check for various functions. We must include pthread.h, # since some functions may be macros. (On the Sequent, we @@ -205,7 +206,7 @@ for flag in $ax_pthread_flags; do # functions on Solaris that doesn't have a non-functional libc stub. # We try pthread_create on general principles. AC_LINK_IFELSE([AC_LANG_PROGRAM([#include - static void routine(void *a) { a = 0; } + static void routine(void *a) { *((int*)a) = 42; } static void *start_routine(void *a) { return a; }], [pthread_t th; pthread_attr_t attr; pthread_create(&th, 0, start_routine, 0); @@ -217,7 +218,7 @@ for flag in $ax_pthread_flags; do []) LIBS="$save_LIBS" - CFLAGS="$save_CFLAGS" + CXXFLAGS="$save_CXXFLAGS" AC_MSG_RESULT($ax_pthread_ok) if test "x$ax_pthread_ok" = xyes; then @@ -233,8 +234,8 @@ fi if test "x$ax_pthread_ok" = xyes; then save_LIBS="$LIBS" LIBS="$PTHREAD_LIBS $LIBS" - save_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS $PTHREAD_CFLAGS" + save_CXXFLAGS="$CXXFLAGS" + CXXFLAGS="$CXXFLAGS $PTHREAD_CFLAGS" # Detect AIX lossage: JOINABLE attribute is called UNDETACHED. AC_MSG_CHECKING([for joinable pthread attribute]) @@ -281,7 +282,7 @@ if test "x$ax_pthread_ok" = xyes; then AC_DEFINE([HAVE_PTHREAD_PRIO_INHERIT], 1, [Have PTHREAD_PRIO_INHERIT.])) LIBS="$save_LIBS" - CFLAGS="$save_CFLAGS" + CXXFLAGS="$save_CXXFLAGS" # More AIX lossage: must compile with xlc_r or cc_r if test x"$GCC" != xyes; then From 95d60410186ed8311014d605ad02171c1fa54de3 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 4 Feb 2012 19:53:03 +0100 Subject: [PATCH 5/5] Fix a busy loop with the connection queue If connecting to a server failed without needing any time for DNS, the connect timer would busy loop over the networks, because a network re-inserted itself into the queue and the timer would try the network again. Fix this by moving the connection queue to a separate instance of std::list when the timer fires. From then on, we just iterate through that list while networks which want to try again add themselves to the "real" connection queue instead. We only have to make sure that any networks that are left in the old connection queue after the timer is done get prepended to the "real" connection queue. Signed-off-by: Uli Schlachter --- src/znc.cpp | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/znc.cpp b/src/znc.cpp index f403d5aa..3bf58a27 100644 --- a/src/znc.cpp +++ b/src/znc.cpp @@ -1827,27 +1827,33 @@ public: protected: virtual void RunJob() { - list& ConnectionQueue = CZNC::Get().GetConnectionQueue(); + list ConnectionQueue; + list& RealConnectionQueue = CZNC::Get().GetConnectionQueue(); - /* We store the end of the queue, so CIRCNetwork::Connect() can add - * itself back to the queue and we wont end up in an infinite loop. */ - list::iterator end = ConnectionQueue.end(); - list::iterator it; + // Problem: If a network can't connect right now because e.g. it + // is throttled, it will re-insert itself into the connection + // queue. However, we must only give each network a single + // chance during this timer run. + // + // Solution: We move the connection queue to our local list at + // the beginning and work from that. + ConnectionQueue.swap(RealConnectionQueue); - for (it = ConnectionQueue.begin(); it != end;) { - CIRCNetwork *pNetwork = *it; - - /* We must erase the network from the queue before we try to connect - * because it may try to add the network to the queue (which would - * fail if we were already in the queue) */ - it = ConnectionQueue.erase(it); + while (!ConnectionQueue.empty()) { + CIRCNetwork *pNetwork = ConnectionQueue.front(); + ConnectionQueue.pop_front(); if (pNetwork->Connect()) { break; } } - if (ConnectionQueue.empty()) { + /* Now re-insert anything that is left in our local list into + * the real connection queue. + */ + RealConnectionQueue.splice(RealConnectionQueue.begin(), ConnectionQueue); + + if (RealConnectionQueue.empty()) { DEBUG("ConnectQueueTimer done"); CZNC::Get().DisableConnectQueue(); }