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.

1 files changed, 62 insertions, 18 deletions

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 <stdlib.h>
 #include <string.h>
 #include <signal.h>
+#include <errno.h>
 #include <pthread.h>
 #include <assert.h>
 
@@ -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:
  */