- working on a potential race condition on the new input module interface.

    See newsgroup posting for details on the issue:
    http://groups.google.com/group/comp.programming.threads/msg/330b9675f17
    a1ad6 I tried some mutex operations but came to the conclusion that
    this does not really help. So I have now switched to plain thread
    cancellation, which so far seems to be OK. Need more practical
    experience with other input modules to make a final decision. Thus I
    leave all code in and have just disabled the problematic code.

4 files changed, 43 insertions, 9 deletions

diff --git a/plugins/imklog/imklog.c b/plugins/imklog/imklog.c
index 157dee73..1c3c79c3 100644
--- a/plugins/imklog/imklog.c
+++ b/plugins/imklog/imklog.c
@@ -46,11 +46,14 @@ MODULE_TYPE_INPUT
 TERM_SYNC_TYPE(eTermSync_SIGNAL)
 
 /* Module static data */
-int dbgPrintSymbols = 0;
 DEF_IMOD_STATIC_DATA
 typedef struct _instanceData {
 } instanceData;
 
+/* configuration settings TODO: move to instance data? */
+int dbgPrintSymbols = 0;
+int symbols_twice = 0;
+
 
 /* Includes. */
 #include <unistd.h>
@@ -87,12 +90,11 @@ static int	kmsg,
 static int	use_syscall = 0,
 		symbol_lookup = 1;
 
-static char	*symfile = (char *) 0,
+static char	*symfile = NULL,
 		log_buffer[LOG_BUFFER_SIZE];
 
 static enum LOGSRC {none, proc, kernel} logsrc;
 
-int symbols_twice = 0;
 
 
 /* Function prototypes. */
diff --git a/plugins/immark/immark.c b/plugins/immark/immark.c
index b7b816b6..973821ad 100644
--- a/plugins/immark/immark.c
+++ b/plugins/immark/immark.c
@@ -68,6 +68,7 @@ typedef struct _instanceData {
  */
 BEGINrunInput
 CODESTARTrunInput
+	thrdBlockTermination(pThrd);
 	/* this is an endless loop - it is terminated when the thread is
 	 * signalled to do so. This, however, is handled by the framework,
 	 * right into the sleep below.
diff --git a/threads.c b/threads.c
index 88b4ce8f..04eea8f1 100644
--- a/threads.c
+++ b/threads.c
@@ -1,3 +1,7 @@
+/* TODO: we should guard the individual thread actions with a mutex. Else, we may
+ * run into race conditions on thread termination.
+ */
+
 /* threads.c
  *
  * This file implements threading support helpers (and maybe the thread object)
@@ -25,7 +29,6 @@
  * A copy of the GPL can be found in the file "COPYING" in this distribution.
  */
 #include "config.h"
-#include "rsyslog.h"
 
 #include <stdlib.h>
 #include <string.h>
@@ -33,6 +36,7 @@
 #include <pthread.h>
 #include <assert.h>
 
+#include "rsyslog.h"
 #include "syslogd.h"
 #include "linkedlist.h"
 #include "threads.h"
@@ -48,18 +52,22 @@ static linkedList_t llThrds;
 
 /* Construct a new thread object
  */
-static rsRetVal thrdConstruct(thrdInfo_t **pThis)
+static rsRetVal thrdConstruct(thrdInfo_t **ppThis)
 {
-	thrdInfo_t *pNew;
+	thrdInfo_t *pThis;
+
+	assert(ppThis != NULL);
 
-	if((pNew = calloc(1, sizeof(thrdInfo_t))) == NULL)
+	if((pThis = calloc(1, sizeof(thrdInfo_t))) == NULL)
 		return RS_RET_OUT_OF_MEMORY;
 
 	/* 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);
 
-	*pThis = pNew;
+	*ppThis = pThis;
 	return RS_RET_OK;
 }
 
@@ -76,6 +84,7 @@ dbgprintf("thrdDestruct, pThis: %lx\n", pThis);
 	if(pThis->bIsActive == 1) {
 		thrdTerminate(pThis);
 	}
+	free(pThis->mutTermOK);
 	free(pThis);
 
 	return RS_RET_OK;
@@ -91,9 +100,15 @@ rsRetVal thrdTerminate(thrdInfo_t *pThis)
 	
 dbgprintf("Terminate thread %lx via method %d\n", pThis->thrdID, pThis->eTermTool);
 	if(pThis->eTermTool == eTermSync_SIGNAL) {
+		/* we first wait for the thread to reach a point in execution where it
+		 * is safe to terminate it.
+		 * TODO: TIMEOUT!
+		 */
+		pthread_mutex_lock(pThis->mutTermOK);
 		pThis->bShallStop = 1;	/* request termination */
 		pthread_kill(pThis->thrdID, SIGUSR2); /* get thread out ouf blocking calls */
 		pthread_join(pThis->thrdID, NULL);
+		pthread_mutex_unlock(pThis->mutTermOK);	 /* cleanup... */
 		/* TODO: TIMEOUT! */
 	} else if(pThis->eTermTool == eTermSync_NONE) {
 		pthread_cancel(pThis->thrdID);
@@ -162,7 +177,7 @@ rsRetVal thrdCreate(rsRetVal (*thrdMain)(thrdInfo_t*), eTermSyncType_t eTermSync
 	assert(thrdMain != NULL);
 
 	CHKiRet(thrdConstruct(&pThis));
-	pThis->eTermTool = eTermSyncType;
+	pThis->eTermTool = eTermSync_NONE; // eTermSyncType; TODO: review
 	pThis->bIsActive = 1;
 	pThis->pUsrThrdMain = thrdMain;
 	pThis->pAfterRun = afterRun;
@@ -233,9 +248,15 @@ thrdSleep(thrdInfo_t *pThis, int iSeconds, int iuSeconds)
 	assert(pThis != NULL);
 	tvSelectTimeout.tv_sec = iSeconds;
 	tvSelectTimeout.tv_usec = iuSeconds; /* micro seconds */
+	thrdUnblockTermination(pThis);
+	/* there may be a race condition if pthread_kill() is called after unblock but
+	 * before the select() is setup. TODO: check and re-eval -- rgerhards, 2007-12-20
+	 */
 	select(0, NULL, NULL, NULL, &tvSelectTimeout);
 	if(pThis->bShallStop)
 		iRet = RS_RET_TERMINATE_NOW;
+	else
+		thrdBlockTermination(pThis);
 	return iRet;
 }
 
diff --git a/threads.h b/threads.h
index 51ae52cf..6bf1e3b1 100644
--- a/threads.h
+++ b/threads.h
@@ -33,6 +33,7 @@ typedef enum eTermSyncType {
 /* the thread object */
 typedef struct thrdInfo {
 	eTermSyncType_t	eTermTool;
+	pthread_mutex_t *mutTermOK;	/* Is it ok to terminate that thread now? */
 	int bIsActive;		/* Is thread running? */
 	int bShallStop;		/* set to 1 if the thread should be stopped ? */
 	rsRetVal (*pUsrThrdMain)(struct thrdInfo*); /* user thread main to be called in new thread */
@@ -67,4 +68,13 @@ void queueDel (msgQueue *q, void **out);
 extern int iMainMsgQueueSize;
 extern msgQueue *pMsgQueue;
 
+
+/* macros (replace inline functions) */
+/*TODO: remove these macros once we now we can live without -- rgerhards, 2007-12-20
+ * #define thrdBlockTermination(pThis) {dbgprintf("lock mutex\n"); pthread_mutex_lock((pThis)->mutTermOK)  ;}
+ * #define thrdUnblockTermination(pThis) {dbgprintf("unlock mutex\n"); pthread_mutex_unlock((pThis)->mutTermOK)  ;}
+ */
+#define thrdBlockTermination(pThis)
+#define thrdUnblockTermination(pThis)
+
 #endif /* #ifndef THREADS_H_INCLUDED */