code review, robustness increase, avoid infinite wait by exit, thread exit and by pipes of closed processes);

use pipe-helpers (TI-structure handling) for all pipe-workers (tclWinConsole, tclWinSerial);

1 files changed, 15 insertions, 109 deletions

diff --git a/win/tclWinSerial.c b/win/tclWinSerial.c
index f55f5f1..f78aa5a 100644
--- a/win/tclWinSerial.c
+++ b/win/tclWinSerial.c
@@ -93,19 +93,12 @@ typedef struct SerialInfo {
 				 * threads. */
     OVERLAPPED osRead;		/* OVERLAPPED structure for read operations. */
     OVERLAPPED osWrite;		/* OVERLAPPED structure for write operations */
+    TclPipeThreadInfo *writeTI;	/* Thread info structure of writer worker. */
     HANDLE writeThread;		/* Handle to writer thread. */
-    int writeThreadExiting;	/* Boolean indicating that thread is exiting. */
     CRITICAL_SECTION csWrite;	/* Writer thread synchronisation. */
     HANDLE evWritable;		/* Manual-reset event to signal when the
 				 * writer thread has finished waiting for the
 				 * current buffer to be written. */
-    HANDLE evStartWriter;	/* Auto-reset event used by the main thread to
-				 * signal when the writer thread should
-				 * attempt to write to the serial. Additionally
-				 * this event used as wait for thread event (init). */
-    HANDLE evStopWriter;	/* Auto-reset event used by the main thread to
-				 * signal when the writer thread should close.
-				 */
     DWORD writeError;		/* An error caused by the last background
 				 * write. Set to 0 if no error has been
 				 * detected. This word is shared with the
@@ -601,7 +594,6 @@ SerialCloseProc(
     int errorCode, result = 0;
     SerialInfo *infoPtr, **nextPtrPtr;
     ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey);
-    DWORD exitCode;
 
     errorCode = 0;
 
@@ -611,71 +603,13 @@ SerialCloseProc(
     }
     serialPtr->validMask &= ~TCL_READABLE;
 
-    if (serialPtr->validMask & TCL_WRITABLE) {
-	/*
-	 * Generally we cannot wait for a pending write operation because it
-	 * may hang due to handshake
-	 *    WaitForSingleObject(serialPtr->evWritable, INFINITE);
-	 */
-
-	/*
-	 * The thread may have already closed on it's own. Check it's exit
-	 * code.
-	 */
-
-	GetExitCodeThread(serialPtr->writeThread, &exitCode);
-
-	if (exitCode == STILL_ACTIVE) {
-	    /*
-	     * Set the stop event so that if the writer thread is blocked in
-	     * SerialWriterThread on WaitForMultipleEvents, it will exit
-	     * cleanly.
-	     */
+    if (serialPtr->writeThread) {
 
-	    SetEvent(serialPtr->evStopWriter);
+    	TclPipeThreadStop(&serialPtr->writeTI, serialPtr->writeThread);
 
-	    /*
-	     * Wait at most 20 milliseconds for the writer thread to close.
-	     */
-
-	    if (WaitForSingleObject(serialPtr->writeThread,
-		    20) == WAIT_TIMEOUT) {
-		/*
-		 * Forcibly terminate the background thread as a last resort.
-		 * Note that we need to guard against terminating the thread
-		 * while it is in the middle of Tcl_ThreadAlert because it
-		 * won't be able to release the notifier lock.
-		 *
-		 * Also note that terminating threads during their initialization or teardown phase
-		 * may result in ntdll.dll's LoaderLock to remain locked indefinitely.
-		 * This causes ntdll.dll's LdrpInitializeThread() to deadlock trying to acquire LoaderLock.
-		 * LdrpInitializeThread() is executed within new threads to perform
-		 * initialization and to execute DllMain() of all loaded dlls.
-		 * As a result, all new threads are deadlocked in their initialization phase and never execute,
-		 * even though CreateThread() reports successful thread creation.
-		 * This results in a very weird process-wide behavior, which is extremely hard to debug.
-		 * 
-		 * THREADS SHOULD NEVER BE TERMINATED. Period.
-		 * 
-		 * But for now, check if thread is exiting, and if so, let it die peacefully.
-		 */
-
-		if ( !serialPtr->writeThreadExiting
-		  || WaitForSingleObject(serialPtr->writeThread, 5000) != WAIT_OBJECT_0
-		) {
-		    Tcl_MutexLock(&serialMutex);
-		    /* BUG: this leaks memory. */
-		    TerminateThread(serialPtr->writeThread, 0);
-		    Tcl_MutexUnlock(&serialMutex);
-		}
-	    }
-	}
-
-	CloseHandle(serialPtr->writeThread);
 	CloseHandle(serialPtr->osWrite.hEvent);
 	CloseHandle(serialPtr->evWritable);
-	CloseHandle(serialPtr->evStartWriter);
-	CloseHandle(serialPtr->evStopWriter);
+	CloseHandle(serialPtr->writeThread);
 	serialPtr->writeThread = NULL;
 
 	PurgeComm(serialPtr->handle, PURGE_TXABORT | PURGE_TXCLEAR);
@@ -1093,7 +1027,7 @@ SerialOutputProc(
 	memcpy(infoPtr->writeBuf, buf, (size_t) toWrite);
 	infoPtr->toWrite = toWrite;
 	ResetEvent(infoPtr->evWritable);
-	SetEvent(infoPtr->evStartWriter);
+	TclPipeThreadSignal(&infoPtr->writeTI);
 	bytesWritten = (DWORD) toWrite;
 
     } else {
@@ -1330,39 +1264,21 @@ static DWORD WINAPI
 SerialWriterThread(
     LPVOID arg)
 {
-    SerialInfo *infoPtr = (SerialInfo *)arg;
-    DWORD bytesWritten, toWrite, waitResult;
+    TclPipeThreadInfo *pipeTI = (TclPipeThreadInfo *)arg;
+    SerialInfo *infoPtr = NULL; /* access info only after success init/wait */
+    DWORD bytesWritten, toWrite;
     char *buf;
     OVERLAPPED myWrite;		/* Have an own OVERLAPPED in this thread. */
-    HANDLE wEvents[2];
-
-    /*
-     * Notify TclWinOpenSerialChannel() that this thread is initialized
-     */
-    SignalObjectAndWait(infoPtr->evStartWriter, infoPtr->evStopWriter, INFINITE, FALSE);
-
-    /*
-     * The stop event takes precedence by being first in the list.
-     */
-
-    wEvents[0] = infoPtr->evStopWriter;
-    wEvents[1] = infoPtr->evStartWriter;
 
     for (;;) {
 	/*
 	 * Wait for the main thread to signal before attempting to write.
 	 */
-
-	waitResult = WaitForMultipleObjects(2, wEvents, FALSE, INFINITE);
-
-	if (waitResult != (WAIT_OBJECT_0 + 1)) {
-	    /*
-	     * The start event was not signaled. It might be the stop event or
-	     * an error, so exit.
-	     */
-
+	if (!TclPipeThreadWaitForSignal(&pipeTI)) {
+	    /* exit */
 	    break;
 	}
+	infoPtr = (SerialInfo *)pipeTI->clientData;
 
 	buf = infoPtr->writeBuf;
 	toWrite = infoPtr->toWrite;
@@ -1426,11 +1342,8 @@ SerialWriterThread(
 	Tcl_MutexUnlock(&serialMutex);
     }
 
-    /*
-     * Inform caller that this thread should not be terminated, since it is about to exit.
-     * See comment in SerialCloseProc() for reasons.
-     */
-    infoPtr->writeThreadExiting = TRUE;
+    /* Worker exit, so inform the main thread or free TI-structure (if owned) */
+    TclPipeThreadExit(&pipeTI);
 
     return 0;
 }
@@ -1505,7 +1418,6 @@ TclWinOpenSerialChannel(
     int permissions)
 {
     SerialInfo *infoPtr;
-    DWORD id;
 
     SerialInit();
 
@@ -1557,15 +1469,9 @@ TclWinOpenSerialChannel(
 
 	infoPtr->osWrite.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
 	infoPtr->evWritable = CreateEvent(NULL, TRUE, TRUE, NULL);
-	infoPtr->evStartWriter = CreateEvent(NULL, FALSE, FALSE, NULL);
-	infoPtr->evStopWriter = CreateEvent(NULL, FALSE, FALSE, NULL);
-	infoPtr->writeThreadExiting = FALSE;
 	infoPtr->writeThread = CreateThread(NULL, 256, SerialWriterThread,
-		infoPtr, 0, &id);
-	/* Wait for thread to initialize (using evStartWriter) */
-	WaitForSingleObject(infoPtr->evStartWriter, 5000);
-	/* Wake-up it to signal we've get an event */
-	SetEvent(infoPtr->evStopWriter);
+		TclPipeThreadCreateTI(&infoPtr->writeTI, infoPtr, 
+			infoPtr->evWritable), 0, NULL);
     }
 
     /*