From e53d91ce666453b114880e0619dd8c4d40072201 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 15 Oct 2009 18:33:33 +0200 Subject: solved a recently introduced race during input thread shutdown This was introduced when we re-enabled non-cancel thread termination a few commits ago. This code has never been released as a tarball, so that is no bugfix for a release but rather a WiP regression fix and thus does not need to be mentioned in the ChangeLog. --- threads.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 18 deletions(-) (limited to 'threads.c') diff --git a/threads.c b/threads.c index a6cbc2ff..b8c19ed2 100644 --- a/threads.c +++ b/threads.c @@ -5,7 +5,7 @@ * * File begun on 2007-12-14 by RGerhards * - * Copyright 2007 Rainer Gerhards and Adiscon GmbH. + * Copyright 2007, 2009 Rainer Gerhards and Adiscon GmbH. * * This file is part of rsyslog. * @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -36,6 +37,7 @@ #include "dirty.h" #include "linkedlist.h" #include "threads.h" +#include "srUtils.h" /* linked list of currently-known threads */ static linkedList_t llThrds; @@ -44,7 +46,8 @@ static linkedList_t llThrds; /* Construct a new thread object */ -static rsRetVal thrdConstruct(thrdInfo_t **ppThis) +static rsRetVal +thrdConstruct(thrdInfo_t **ppThis) { DEFiRet; thrdInfo_t *pThis; @@ -52,13 +55,8 @@ static rsRetVal thrdConstruct(thrdInfo_t **ppThis) assert(ppThis != NULL); CHKmalloc(pThis = calloc(1, sizeof(thrdInfo_t))); - - /* OK, we got the element, now initialize members that should - * not be zero-filled. - */ - pThis->mutTermOK = (pthread_mutex_t *) malloc (sizeof (pthread_mutex_t)); - pthread_mutex_init (pThis->mutTermOK, NULL); - + pthread_mutex_init(&pThis->mutThrd, NULL); + pthread_cond_init(&pThis->condThrdTerm, NULL); *ppThis = pThis; finalize_it: @@ -78,13 +76,54 @@ static rsRetVal thrdDestruct(thrdInfo_t *pThis) if(pThis->bIsActive == 1) { thrdTerminate(pThis); } - free(pThis->mutTermOK); + pthread_mutex_destroy(&pThis->mutThrd); + pthread_cond_destroy(&pThis->condThrdTerm); free(pThis); RETiRet; } +/* terminate a thread via the non-cancel interface + * This is a separate function as it involves a bit more of code. + * rgerhads, 2009-10-15 + */ +static inline rsRetVal +thrdTerminateNonCancel(thrdInfo_t *pThis) +{ + struct timespec tTimeout; + int ret; + DEFiRet; + assert(pThis != NULL); + + DBGPRINTF("request term via SIGTTIN for input thread 0x%x\n", (unsigned) pThis->thrdID); + pThis->bShallStop = TRUE; + do { + d_pthread_mutex_lock(&pThis->mutThrd); + pthread_kill(pThis->thrdID, SIGTTIN); + timeoutComp(&tTimeout, 10); /* a fixed 10ms timeout, do after lock (may take long!) */ + ret = d_pthread_cond_timedwait(&pThis->condThrdTerm, &pThis->mutThrd, &tTimeout); + d_pthread_mutex_unlock(&pThis->mutThrd); + if(Debug) { + if(ret == ETIMEDOUT) { + dbgprintf("input thread term: had a timeout waiting on thread termination\n"); + } else if(ret == 0) { + dbgprintf("input thread term: thread returned normally and is terminated\n"); + } else { + char errStr[1024]; + int err = errno; + rs_strerror_r(err, errStr, sizeof(errStr)); + dbgprintf("input thread term: cond_wait returned with error %d: %s\n", + err, errStr); + } + } + } while(pThis->bIsActive); + DBGPRINTF("non-cancel input thread termination succeeded for thread 0x%x\n", (unsigned) pThis->thrdID); + + RETiRet; +} + + /* terminate a thread gracefully. */ rsRetVal thrdTerminate(thrdInfo_t *pThis) @@ -95,13 +134,11 @@ rsRetVal thrdTerminate(thrdInfo_t *pThis) if(pThis->bNeedsCancel) { DBGPRINTF("request term via canceling for input thread 0x%x\n", (unsigned) pThis->thrdID); pthread_cancel(pThis->thrdID); + pThis->bIsActive = 0; } else { - - DBGPRINTF("request term via SIGTTIN for input thread 0x%x\n", (unsigned) pThis->thrdID); - pthread_kill(pThis->thrdID, SIGTTIN); + thrdTerminateNonCancel(pThis); } pthread_join(pThis->thrdID, NULL); /* wait for input thread to complete */ - pThis->bIsActive = 0; /* call cleanup function, if any */ if(pThis->pAfterRun != NULL) @@ -152,6 +189,16 @@ static void* thrdStarter(void *arg) iRet = pThis->pUsrThrdMain(pThis); dbgprintf("thrdStarter: usrThrdMain 0x%lx returned with iRet %d, exiting now.\n", (unsigned long) pThis->thrdID, iRet); + + /* signal master control that we exit (we do the mutex lock mostly to + * keep the thread debugger happer, it would not really be necessary with + * the logic we employ...) + */ + pThis->bIsActive = 0; + d_pthread_mutex_lock(&pThis->mutThrd); + pthread_cond_signal(&pThis->condThrdTerm); + d_pthread_mutex_unlock(&pThis->mutThrd); + ENDfunc pthread_exit(0); } @@ -198,9 +245,7 @@ rsRetVal thrdInit(void) rsRetVal thrdExit(void) { DEFiRet; - iRet = llDestroy(&llThrds); - RETiRet; } @@ -229,6 +274,5 @@ thrdSleep(thrdInfo_t *pThis, int iSeconds, int iuSeconds) } -/* - * vi:set ai: +/* vi:set ai: */ -- cgit