From d124e69932241980f1f75dc94c7abbf4981ccb95 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 5 Apr 2017 10:40:51 +0000 Subject: small review: rewritten using already available event handles, additionally prevents infinite waits (using timeout 5000ms); --- win/tclWinConsole.c | 55 +++++++++++++++++++--------------- win/tclWinPipe.c | 85 +++++++++++++++++++++++++++++------------------------ win/tclWinSerial.c | 34 ++++++++++----------- 3 files changed, 94 insertions(+), 80 deletions(-) diff --git a/win/tclWinConsole.c b/win/tclWinConsole.c index 5c0b43b..42bc56f 100644 --- a/win/tclWinConsole.c +++ b/win/tclWinConsole.c @@ -51,14 +51,14 @@ TCL_DECLARE_MUTEX(consoleMutex) typedef struct ConsoleThreadInfo { HANDLE thread; /* Handle to reader or writer thread. */ - HANDLE threadInitialized; /* Manual-reset event to signal that thread has been initialized. */ int threadExiting; /* Boolean indicating that thread is exiting. */ HANDLE readyEvent; /* Manual-reset event to signal _to_ the main * thread when the worker thread has finished * waiting for its normal work to happen. */ HANDLE startEvent; /* Auto-reset event used by the main thread to * signal when the thread should attempt to do - * its normal work. */ + * its normal work. Additionally this event + * used as wait for thread event (init phase). */ HANDLE stopEvent; /* Auto-reset event used by the main thread to * signal when the thread should exit. */ } ConsoleThreadInfo; @@ -538,12 +538,10 @@ StartChannelThread( threadInfoPtr->readyEvent = CreateEvent(NULL, TRUE, TRUE, NULL); threadInfoPtr->startEvent = CreateEvent(NULL, FALSE, FALSE, NULL); - threadInfoPtr->threadInitialized = CreateEvent(NULL, TRUE, FALSE, NULL); threadInfoPtr->threadExiting = FALSE; threadInfoPtr->stopEvent = CreateEvent(NULL, FALSE, FALSE, NULL); threadInfoPtr->thread = CreateThread(NULL, 256, threadProc, infoPtr, 0, &id); - WaitForSingleObject(threadInfoPtr->threadInitialized, INFINITE); /* wait for thread to initialize */ SetThreadPriority(threadInfoPtr->thread, THREAD_PRIORITY_HIGHEST); } @@ -592,14 +590,14 @@ StopChannelThread( * But for now, check if thread is exiting, and if so, let it die peacefully. */ - Tcl_MutexLock(&consoleMutex); - if ( threadInfoPtr->threadExiting ) { - WaitForSingleObject(threadInfoPtr->thread, INFINITE); - } else { - /* BUG: this leaks memory. */ - TerminateThread(threadInfoPtr->thread, 0); + if ( !threadInfoPtr->threadExiting + || WaitForSingleObject(threadInfoPtr->thread, 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&consoleMutex); + /* BUG: this leaks memory. */ + TerminateThread(threadInfoPtr->thread, 0); + Tcl_MutexUnlock(&consoleMutex); } - Tcl_MutexUnlock(&consoleMutex); } } @@ -609,7 +607,6 @@ StopChannelThread( */ CloseHandle(threadInfoPtr->thread); - CloseHandle(threadInfoPtr->threadInitialized); CloseHandle(threadInfoPtr->readyEvent); CloseHandle(threadInfoPtr->startEvent); CloseHandle(threadInfoPtr->stopEvent); @@ -1209,9 +1206,10 @@ ConsoleReaderThread( HANDLE wEvents[2]; /* - * Notify StartChannelThread() that this thread is initialized + * Notify caller (using startEvent) that this thread is initialized */ - SetEvent(threadInfo->threadInitialized); + SetEvent(threadInfo->startEvent); + SuspendThread(threadInfo->thread); /* until main thread get an event */ /* * The first event takes precedence. @@ -1282,12 +1280,10 @@ ConsoleReaderThread( } /* - * Inform StopChannelThread() that this thread should not be terminated, since it is about to exit. + * Inform caller that this thread should not be terminated, since it is about to exit. * See comment in StopChannelThread() for reasons. */ - Tcl_MutexLock(&consoleMutex); threadInfo->threadExiting = TRUE; - Tcl_MutexUnlock(&consoleMutex); return 0; } @@ -1323,9 +1319,10 @@ ConsoleWriterThread( HANDLE wEvents[2]; /* - * Notify StartChannelThread() that this thread is initialized + * Notify caller (using startEvent) that this thread is initialized */ - SetEvent(threadInfo->threadInitialized); + SetEvent(threadInfo->startEvent); + SuspendThread(threadInfo->thread); /* until main thread get an event */ /* * The first event takes precedence. @@ -1393,12 +1390,10 @@ ConsoleWriterThread( } /* - * Inform StopChannelThread() that this thread should not be terminated, since it is about to exit. + * Inform caller that this thread should not be terminated, since it is about to exit. * See comment in StopChannelThread() for reasons. */ - Tcl_MutexLock(&consoleMutex); threadInfo->threadExiting = TRUE; - Tcl_MutexUnlock(&consoleMutex); return 0; } @@ -1429,7 +1424,8 @@ TclWinOpenConsoleChannel( { char encoding[4 + TCL_INTEGER_SPACE]; ConsoleInfo *infoPtr; - DWORD modes; + DWORD modes, wEventsCnt = 0; + HANDLE wEvents[2], wEventsPtr = wEvents; ConsoleInit(); @@ -1471,12 +1467,25 @@ TclWinOpenConsoleChannel( modes |= ENABLE_LINE_INPUT; SetConsoleMode(infoPtr->handle, modes); StartChannelThread(infoPtr, &infoPtr->reader, ConsoleReaderThread); + wEvents[wEventsCnt++] = infoPtr->reader.startEvent; } if (permissions & TCL_WRITABLE) { StartChannelThread(infoPtr, &infoPtr->writer, ConsoleWriterThread); + wEvents[wEventsCnt++] = infoPtr->writer.startEvent; } + /* + * Wait for both threads to initialize (using theirs startEvent) + */ + if (wEventsCnt) { + WaitForMultipleObjects(wEventsCnt, wEvents, TRUE, 5000); + /* Resume both waiting threads */ + if (infoPtr->reader.thread) + ResumeThread(infoPtr->reader.thread); + if (infoPtr->writer.thread) + ResumeThread(infoPtr->writer.thread); + } /* * Files have default translation of AUTO and ^Z eof char, which means * that a ^Z will be accepted as EOF when reading. diff --git a/win/tclWinPipe.c b/win/tclWinPipe.c index 9677792..48dcc25 100644 --- a/win/tclWinPipe.c +++ b/win/tclWinPipe.c @@ -111,10 +111,8 @@ typedef struct PipeInfo { * threads. */ HANDLE writeThread; /* Handle to writer thread. */ HANDLE readThread; /* Handle to reader thread. */ - HANDLE writeThreadInitialized; /* Manual-reset event to signal that writer thread has been initialized */ - HANDLE readThreadInitialized; /* Manual-reset event to signal that reader thread has been initialized */ - int writeThreadExiting; /* Boolean indicating that write thread is exiting */ - int readThreadExiting; /* Boolean indicating that read thread is exiting */ + int writeThreadExiting; /* Boolean indicating that write thread is exiting */ + int readThreadExiting; /* Boolean indicating that read thread is exiting */ HANDLE writable; /* Manual-reset event to signal when the * writer thread has finished waiting for the * current buffer to be written. */ @@ -123,12 +121,14 @@ typedef struct PipeInfo { * input. */ HANDLE startWriter; /* Auto-reset event used by the main thread to * signal when the writer thread should - * attempt to write to the pipe. */ + * attempt to write to the pipe. Additionally + * this event used as wait for thread event (init). */ HANDLE stopWriter; /* Manual-reset event used to alert the reader * thread to fall-out and exit */ HANDLE startReader; /* Auto-reset event used by the main thread to * signal when the reader thread should - * attempt to read from the pipe. */ + * attempt to read from the pipe. Additionally + * this event used as wait for thread event (init). */ HANDLE stopReader; /* Manual-reset event used to alert the reader * thread to fall-out and exit */ DWORD writeError; /* An error caused by the last background @@ -1575,7 +1575,8 @@ TclpCreateCommandChannel( Tcl_Pid *pidPtr) /* An array of process identifiers. */ { char channelName[16 + TCL_INTEGER_SPACE]; - DWORD id; + DWORD id, wEventsCnt = 0; + HANDLE wEvents[2]; PipeInfo *infoPtr = ckalloc(sizeof(PipeInfo)); PipeInit(); @@ -1605,13 +1606,12 @@ TclpCreateCommandChannel( infoPtr->readable = CreateEvent(NULL, TRUE, TRUE, NULL); infoPtr->startReader = CreateEvent(NULL, FALSE, FALSE, NULL); infoPtr->stopReader = CreateEvent(NULL, TRUE, FALSE, NULL); - infoPtr->readThreadInitialized = CreateEvent(NULL, TRUE, FALSE, NULL); infoPtr->readThreadExiting = FALSE; infoPtr->readThread = CreateThread(NULL, 256, PipeReaderThread, infoPtr, 0, &id); - WaitForSingleObject(infoPtr->readThreadInitialized, INFINITE); /* wait for thread to initialize */ SetThreadPriority(infoPtr->readThread, THREAD_PRIORITY_HIGHEST); infoPtr->validMask |= TCL_READABLE; + wEvents[wEventsCnt++] = infoPtr->startReader; } else { infoPtr->readThread = 0; } @@ -1623,13 +1623,26 @@ TclpCreateCommandChannel( infoPtr->writable = CreateEvent(NULL, TRUE, TRUE, NULL); infoPtr->startWriter = CreateEvent(NULL, FALSE, FALSE, NULL); infoPtr->stopWriter = CreateEvent(NULL, TRUE, FALSE, NULL); - infoPtr->writeThreadInitialized = CreateEvent(NULL, TRUE, FALSE, NULL); infoPtr->writeThreadExiting = FALSE; infoPtr->writeThread = CreateThread(NULL, 256, PipeWriterThread, infoPtr, 0, &id); - WaitForSingleObject(infoPtr->writeThreadInitialized, INFINITE); /* wait for thread to initialize */ SetThreadPriority(infoPtr->readThread, THREAD_PRIORITY_HIGHEST); infoPtr->validMask |= TCL_WRITABLE; + wEvents[wEventsCnt++] = infoPtr->startWriter; + } else { + infoPtr->writeThread = 0; + } + + /* + * Wait for both threads to initialize (using theirs start-events) + */ + if (wEventsCnt) { + WaitForMultipleObjects(wEventsCnt, wEvents, TRUE, 5000); + /* Resume both waiting threads */ + if (infoPtr->readThread) + ResumeThread(infoPtr->readThread); + if (infoPtr->writeThread) + ResumeThread(infoPtr->writeThread); } /* @@ -1878,20 +1891,18 @@ PipeClose2Proc( * But for now, check if thread is exiting, and if so, let it die peacefully. */ - Tcl_MutexLock(&pipeMutex); - - if ( pipePtr->readThreadExiting ) { - WaitForSingleObject(pipePtr->readThread, INFINITE); - } else { - /* BUG: this leaks memory */ - TerminateThread(pipePtr->readThread, 0); + if ( !pipePtr->readThreadExiting + || WaitForSingleObject(pipePtr->readThread, 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&pipeMutex); + /* BUG: this leaks memory */ + TerminateThread(pipePtr->readThread, 0); + Tcl_MutexUnlock(&pipeMutex); } - Tcl_MutexUnlock(&pipeMutex); } } CloseHandle(pipePtr->readThread); - CloseHandle(pipePtr->readThreadInitialized); CloseHandle(pipePtr->readable); CloseHandle(pipePtr->startReader); CloseHandle(pipePtr->stopReader); @@ -1978,20 +1989,18 @@ PipeClose2Proc( * But for now, check if thread is exiting, and if so, let it die peacefully. */ - Tcl_MutexLock(&pipeMutex); - - if ( pipePtr->writeThreadExiting ) { - WaitForSingleObject(pipePtr->writeThread, INFINITE); - } else { - /* BUG: this leaks memory */ - TerminateThread(pipePtr->writeThread, 0); + if ( !pipePtr->writeThreadExiting + || WaitForSingleObject(pipePtr->writeThread, 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&pipeMutex); + /* BUG: this leaks memory */ + TerminateThread(pipePtr->writeThread, 0); + Tcl_MutexUnlock(&pipeMutex); } - Tcl_MutexUnlock(&pipeMutex); } } CloseHandle(pipePtr->writeThread); - CloseHandle(pipePtr->writeThreadInitialized); CloseHandle(pipePtr->writable); CloseHandle(pipePtr->startWriter); CloseHandle(pipePtr->stopWriter); @@ -2868,9 +2877,10 @@ PipeReaderThread( DWORD waitResult; /* - * Let TclpCreateCommandChannel() know that this thread has been initialized + * Notify caller that this thread has been initialized */ - SetEvent(infoPtr->readThreadInitialized); + SetEvent(infoPtr->startReader); + SuspendThread(infoPtr->readThread); /* until main thread get an event */ wEvents[0] = infoPtr->stopReader; wEvents[1] = infoPtr->startReader; @@ -2965,12 +2975,10 @@ PipeReaderThread( } /* - * Inform PipeClose2Proc() that this thread should not be terminated, since it is about to exit. + * Inform caller that this thread should not be terminated, since it is about to exit. * See comment in PipeClose2Proc() for reasons. */ - Tcl_MutexLock(&pipeMutex); infoPtr->readThreadExiting = TRUE; - Tcl_MutexUnlock(&pipeMutex); return 0; } @@ -3005,9 +3013,10 @@ PipeWriterThread( DWORD waitResult; /* - * Let TclpCreateCommandChannel() know that this thread has been initialized + * Notify caller that this thread has been initialized */ - SetEvent(infoPtr->writeThreadInitialized); + SetEvent(infoPtr->startWriter); + SuspendThread(infoPtr->writeThread); /* until main thread get an event */ wEvents[0] = infoPtr->stopWriter; wEvents[1] = infoPtr->startWriter; @@ -3076,12 +3085,10 @@ PipeWriterThread( } /* - * Inform PipeClose2Proc() that this thread should not be terminated, since it is about to exit. + * Inform caller that this thread should not be terminated, since it is about to exit. * See comment in PipeClose2Proc() for reasons. */ - Tcl_MutexLock(&pipeMutex); infoPtr->writeThreadExiting = TRUE; - Tcl_MutexUnlock(&pipeMutex); return 0; } diff --git a/win/tclWinSerial.c b/win/tclWinSerial.c index 3c6a3cc..fa135ab 100644 --- a/win/tclWinSerial.c +++ b/win/tclWinSerial.c @@ -94,15 +94,15 @@ typedef struct SerialInfo { OVERLAPPED osRead; /* OVERLAPPED structure for read operations. */ OVERLAPPED osWrite; /* OVERLAPPED structure for write operations */ HANDLE writeThread; /* Handle to writer thread. */ - HANDLE writeThreadInitialized; /* Manual-reset event to signal that thread has been initialized. */ - int writeThreadExiting; /* Boolean indicating that thread is exiting. */ + 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. */ + * 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. */ @@ -660,20 +660,18 @@ SerialCloseProc( * But for now, check if thread is exiting, and if so, let it die peacefully. */ - Tcl_MutexLock(&serialMutex); - - if ( serialPtr->writeThreadExiting ) { - WaitForSingleObject(serialPtr->writeThread, INFINITE); - } else { - /* BUG: this leaks memory. */ - TerminateThread(serialPtr->writeThread, 0); + if ( !serialPtr->writeThreadExiting + || WaitForSingleObject(serialPtr->writeThread, 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&serialMutex); + /* BUG: this leaks memory. */ + TerminateThread(serialPtr->writeThread, 0); + Tcl_MutexUnlock(&serialMutex); } - Tcl_MutexUnlock(&serialMutex); } } CloseHandle(serialPtr->writeThread); - CloseHandle(serialPtr->writeThreadInitialized); CloseHandle(serialPtr->osWrite.hEvent); CloseHandle(serialPtr->evWritable); CloseHandle(serialPtr->evStartWriter); @@ -1341,7 +1339,8 @@ SerialWriterThread( /* * Notify TclWinOpenSerialChannel() that this thread is initialized */ - SetEvent(infoPtr->writeThreadInitialized); + SetEvent(infoPtr->evStartWriter); + SuspendThread(infoPtr->writeThread); /* until main thread get an event */ /* * The stop event takes precedence by being first in the list. @@ -1429,12 +1428,10 @@ SerialWriterThread( } /* - * Inform SerialCloseProc() that this thread should not be terminated, since it is about to exit. + * Inform caller that this thread should not be terminated, since it is about to exit. * See comment in SerialCloseProc() for reasons. */ - Tcl_MutexLock(&serialMutex); infoPtr->writeThreadExiting = TRUE; - Tcl_MutexUnlock(&serialMutex); return 0; } @@ -1563,11 +1560,12 @@ TclWinOpenSerialChannel( infoPtr->evWritable = CreateEvent(NULL, TRUE, TRUE, NULL); infoPtr->evStartWriter = CreateEvent(NULL, FALSE, FALSE, NULL); infoPtr->evStopWriter = CreateEvent(NULL, FALSE, FALSE, NULL); - infoPtr->writeThreadInitialized = CreateEvent(NULL, TRUE, FALSE, NULL); infoPtr->writeThreadExiting = FALSE; infoPtr->writeThread = CreateThread(NULL, 256, SerialWriterThread, infoPtr, 0, &id); - WaitForSingleObject(infoPtr->writeThreadInitialized, INFINITE); /* wait for thread to initialize */ + /* Wait for thread to initialize (using evStartWriter) */ + WaitForSingleObject(infoPtr->evStartWriter, 5000); + ResumeThread(infoPtr->writeThread); } /* -- cgit v0.12