Index: win/tclWinConsole.c ================================================================== --- win/tclWinConsole.c +++ win/tclWinConsole.c @@ -49,16 +49,18 @@ * threads. */ typedef struct ConsoleThreadInfo { HANDLE thread; /* Handle to reader or writer thread. */ + 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; /* @@ -534,10 +536,11 @@ { DWORD id; threadInfoPtr->readyEvent = CreateEvent(NULL, TRUE, TRUE, NULL); threadInfoPtr->startEvent = CreateEvent(NULL, FALSE, FALSE, NULL); + threadInfoPtr->threadExiting = FALSE; threadInfoPtr->stopEvent = CreateEvent(NULL, FALSE, FALSE, NULL); threadInfoPtr->thread = CreateThread(NULL, 256, threadProc, infoPtr, 0, &id); SetThreadPriority(threadInfoPtr->thread, THREAD_PRIORITY_HIGHEST); } @@ -570,16 +573,33 @@ /* * 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. */ - Tcl_MutexLock(&consoleMutex); - /* BUG: this leaks memory. */ - TerminateThread(threadInfoPtr->thread, 0); - Tcl_MutexUnlock(&consoleMutex); + if ( !threadInfoPtr->threadExiting + || WaitForSingleObject(threadInfoPtr->thread, 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&consoleMutex); + /* BUG: this leaks memory. */ + TerminateThread(threadInfoPtr->thread, 0); + Tcl_MutexUnlock(&consoleMutex); + } } } /* * Close all the handles associated with the thread, and set the thread @@ -806,11 +826,11 @@ const char *buf, /* The data buffer. */ int toWrite, /* How many bytes to write? */ int *errorCode) /* Where to store error code. */ { ConsoleInfo *infoPtr = instanceData; - ConsoleThreadInfo *threadInfo = &infoPtr->reader; + ConsoleThreadInfo *threadInfo = &infoPtr->writer; DWORD bytesWritten, timeout; *errorCode = 0; timeout = (infoPtr->flags & CONSOLE_ASYNC) ? 0 : INFINITE; if (WaitForSingleObject(threadInfo->readyEvent,timeout) == WAIT_TIMEOUT) { @@ -1183,10 +1203,15 @@ HANDLE *handle = infoPtr->handle; ConsoleThreadInfo *threadInfo = &infoPtr->reader; DWORD waitResult; HANDLE wEvents[2]; + /* + * Notify caller (using startEvent) that this thread is initialized + */ + SignalObjectAndWait(threadInfo->startEvent, threadInfo->stopEvent, INFINITE, FALSE); + /* * The first event takes precedence. */ wEvents[0] = threadInfo->stopEvent; @@ -1251,10 +1276,16 @@ Tcl_ThreadAlert(infoPtr->threadId); } Tcl_MutexUnlock(&consoleMutex); } + /* + * Inform caller that this thread should not be terminated, since it is about to exit. + * See comment in StopChannelThread() for reasons. + */ + threadInfo->threadExiting = TRUE; + return 0; } /* *---------------------------------------------------------------------- @@ -1284,10 +1315,15 @@ ConsoleThreadInfo *threadInfo = &infoPtr->writer; DWORD count, toWrite, waitResult; char *buf; HANDLE wEvents[2]; + /* + * Notify caller (using startEvent) that this thread is initialized + */ + SignalObjectAndWait(threadInfo->startEvent, threadInfo->stopEvent, INFINITE, FALSE); + /* * The first event takes precedence. */ wEvents[0] = threadInfo->stopEvent; @@ -1349,10 +1385,16 @@ Tcl_ThreadAlert(infoPtr->threadId); } Tcl_MutexUnlock(&consoleMutex); } + /* + * Inform caller that this thread should not be terminated, since it is about to exit. + * See comment in StopChannelThread() for reasons. + */ + threadInfo->threadExiting = TRUE; + return 0; } /* *---------------------------------------------------------------------- @@ -1378,11 +1420,12 @@ char *channelName, int permissions) { char encoding[4 + TCL_INTEGER_SPACE]; ConsoleInfo *infoPtr; - DWORD modes; + DWORD modes, wEventsCnt = 0; + HANDLE wEvents[2], wEventsPtr = wEvents; ConsoleInit(); /* * See if a channel with this handle already exists. @@ -1420,16 +1463,29 @@ GetConsoleMode(infoPtr->handle, &modes); modes &= ~(ENABLE_WINDOW_INPUT | ENABLE_MOUSE_INPUT); 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, we've get the events */ + if (infoPtr->reader.thread) + SetEvent(infoPtr->reader.stopEvent); + if (infoPtr->writer.thread) + SetEvent(infoPtr->writer.stopEvent); + } /* * Files have default translation of AUTO and ^Z eof char, which means * that a ^Z will be accepted as EOF when reading. */ Index: win/tclWinInit.c ================================================================== --- win/tclWinInit.c +++ win/tclWinInit.c @@ -74,10 +74,19 @@ #endif #ifndef PROCESSOR_ARCHITECTURE_UNKNOWN #define PROCESSOR_ARCHITECTURE_UNKNOWN 0xFFFF #endif + +/* + * Windows version dependend functions + */ +static TclWinProcs _tclWinProcs = { + NULL +}; +TclWinProcs *tclWinProcs = &_tclWinProcs; + /* * The following arrays contain the human readable strings for the Windows * platform and processor values. */ @@ -130,10 +139,11 @@ void TclpInitPlatform(void) { WSADATA wsaData; WORD wVersionRequested = MAKEWORD(2, 2); + HINSTANCE hInstance; tclPlatform = TCL_PLATFORM_WINDOWS; /* * Initialize the winsock library. On Windows XP and higher this @@ -148,10 +158,20 @@ * invoked. */ TclWinInit(GetModuleHandle(NULL)); #endif + + /* + * Fill available functions depending on windows version + */ + hInstance = LoadLibraryW(L"kernel32"); + if (hInstance != NULL) { + _tclWinProcs.cancelSynchronousIo = + (BOOL (WINAPI *)(HANDLE)) GetProcAddress(hInstance, + "CancelSynchronousIo"); + } } /* *------------------------------------------------------------------------- * Index: win/tclWinInt.h ================================================================== --- win/tclWinInt.h +++ win/tclWinInt.h @@ -29,10 +29,19 @@ void *esp; int status; } TCLEXCEPTION_REGISTRATION; #endif +/* + * Windows version dependend functions + */ +typedef struct TclWinProcs { + BOOL (WINAPI *cancelSynchronousIo)(HANDLE); +} TclWinProcs; + +MODULE_SCOPE TclWinProcs *tclWinProcs; + /* * Some versions of Borland C have a define for the OSVERSIONINFO for * Win32s and for NT, but not for Windows 95. * Define VER_PLATFORM_WIN32_CE for those without newer headers. */ Index: win/tclWinPipe.c ================================================================== --- win/tclWinPipe.c +++ win/tclWinPipe.c @@ -88,10 +88,329 @@ */ /* * This structure describes per-instance data for a pipe based channel. */ + +typedef struct PipeThreadInfo { + HANDLE evControl; /* Auto-reset event used by the main thread to + * signal when the pipe thread should attempt + * to do read/write operation. Additionally + * used as signal to stop (state set to -1) */ + volatile LONG state; /* Indicates current state of the thread */ + ClientData clientData; /* Referenced data of the main thread */ +} PipeThreadInfo; + + +/* If pipe-workers will use some tcl subsystem, we can use ckalloc without + * more overhead for finalize thread (should be executed anyway) + * + * #define _PTI_USE_CKALLOC 1 + */ + +/* + * State of the pipe-worker. + * + * State PTI_STATE_STOP possible from idle state only, worker owns TI structure. + * Otherwise PTI_STATE_END used (main thread hold ownership of the TI). + */ + +#define PTI_STATE_IDLE 0 /* idle or not yet initialzed */ +#define PTI_STATE_WORK 1 /* in work */ +#define PTI_STATE_STOP 2 /* thread should stop work (owns TI structure) */ +#define PTI_STATE_END 4 /* thread should stop work (worker is busy) */ +#define PTI_STATE_DOWN 8 /* worker is down */ + + +PipeThreadInfo * +PipeThreadCreateTI( + PipeThreadInfo **pipeTIPtr, + ClientData clientData) +{ + PipeThreadInfo *pipeTI; +#ifndef _PTI_USE_CKALLOC + pipeTI = malloc(sizeof(PipeThreadInfo)); +#else + pipeTI = ckalloc(sizeof(PipeThreadInfo)); +#endif + pipeTI->evControl = CreateEvent(NULL, FALSE, FALSE, NULL); + pipeTI->state = PTI_STATE_IDLE; + pipeTI->clientData = clientData; + return (*pipeTIPtr = pipeTI); +} + +int +PipeThreadWaitForSignal( + PipeThreadInfo **pipeTIPtr) +{ + PipeThreadInfo *pipeTI = *pipeTIPtr; + LONG state; + DWORD waitResult; + + if (!pipeTI) { + return 0; + } + /* + * Wait for the main thread to signal before attempting to do the work. + */ + + /* reset work state of thread (idle/waiting) */ + if ((state = InterlockedCompareExchange(&pipeTI->state, + PTI_STATE_IDLE, PTI_STATE_WORK)) & (PTI_STATE_STOP|PTI_STATE_END)) { + /* end of work, check the owner of structure */ + goto end; + } + /* entering wait */ + waitResult = WaitForSingleObject(pipeTI->evControl, INFINITE); + + if (waitResult != WAIT_OBJECT_0) { + + /* + * The control event was not signaled, so end of work (unexpected + * behaviour, main thread can be dead?). + */ + goto end; + } + + /* try to set work state of thread */ + if ((state = InterlockedCompareExchange(&pipeTI->state, + PTI_STATE_WORK, PTI_STATE_IDLE)) & (PTI_STATE_STOP|PTI_STATE_END)) { + /* end of work */ + goto end; + } + + /* signaled to work */ + return 1; + +end: + /* end of work, check the owner of the TI structure */ + if (state != PTI_STATE_STOP) { + *pipeTIPtr = NULL; + } + return 0; +} + +static inline void +PipeThreadSignal( + PipeThreadInfo **pipeTIPtr) +{ + PipeThreadInfo *pipeTI = *pipeTIPtr; + if (pipeTI) { + SetEvent(pipeTI->evControl); + } +} + +int +PipeThreadStopSignal( + PipeThreadInfo **pipeTIPtr) +{ + PipeThreadInfo *pipeTI = *pipeTIPtr; + HANDLE evControl; + int state; + + if (!pipeTI) { + return 1; + } + evControl = pipeTI->evControl; + switch ( + (state = InterlockedCompareExchange(&pipeTI->state, + PTI_STATE_STOP, PTI_STATE_IDLE)) + ) { + + case PTI_STATE_IDLE: + + /* Thread was idle/waiting, notify it goes teardown */ + SetEvent(evControl); + + *pipeTIPtr = NULL; + + case PTI_STATE_DOWN: + + return 1; + + default: + /* + * Thread works currently, we should try to end it, own the TI structure + * (because of possible sharing the joint structures with thread) + */ + InterlockedExchange(&pipeTI->state, PTI_STATE_END); + break; + } + + return 0; +} + +void +PipeThreadStop( + PipeThreadInfo **pipeTIPtr, + HANDLE hThread) +{ + PipeThreadInfo *pipeTI = *pipeTIPtr; + HANDLE evControl; + int state; + + if (!pipeTI) { + return; + } + pipeTI = *pipeTIPtr; + evControl = pipeTI->evControl; + /* + * Try to sane stop the pipe worker, corresponding its current state + */ + switch ( + (state = InterlockedCompareExchange(&pipeTI->state, + PTI_STATE_STOP, PTI_STATE_IDLE)) + ) { + + case PTI_STATE_IDLE: + + /* Thread was idle/waiting, notify it goes teardown */ + SetEvent(evControl); + + /* we don't need to wait for it at all, thread frees himself (owns the TI structure) */ + pipeTI = NULL; + break; + + case PTI_STATE_STOP: + /* already stopped, thread frees himself (owns the TI structure) */ + pipeTI = NULL; + break; + case PTI_STATE_DOWN: + /* Thread already down (?), do nothing */ + + /* we don't need to wait for it, but we should free pipeTI */ + hThread = NULL; + break; + + /* case PTI_STATE_WORK: */ + default: + /* + * Thread works currently, we should try to end it, own the TI structure + * (because of possible sharing the joint structures with thread) + */ + InterlockedExchange(&pipeTI->state, PTI_STATE_END); + break; + } + + if (pipeTI && hThread) { + DWORD exitCode; + + /* + * The thread may already have closed on its own. Check its exit + * code. + */ + + GetExitCodeThread(hThread, &exitCode); + + if (exitCode == STILL_ACTIVE) { + /* + * Set the stop event so that if the pipe thread is blocked + * somewhere, it may hereafter sane exit cleanly. + */ + + SetEvent(evControl); + + /* + * Cancel all sync-IO of this thread (may be blocked there). + */ + if (tclWinProcs->cancelSynchronousIo) { + tclWinProcs->cancelSynchronousIo(hThread); + } + + /* + * Wait at most 20 milliseconds for the reader thread to + * close (regarding TIP#398-fast-exit). + */ + + /* if we want TIP#398-fast-exit. */ + if (WaitForSingleObject(hThread, + TclInExit() ? 0 : 20) == WAIT_TIMEOUT) { + + /* + * The thread must be blocked waiting for the pipe to + * become readable in ReadFile(). There isn't a clean way + * to exit the thread from this condition. We should + * terminate the child process instead to get the reader + * thread to fall out of ReadFile with a FALSE. (below) is + * not the correct way to do this, but will stay here + * until a better solution is found. + * + * 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 ( pipeTI->state != PTI_STATE_DOWN + && WaitForSingleObject(hThread, + TclInExit() ? 0 : 5000) != WAIT_OBJECT_0 + ) { + Tcl_MutexLock(&pipeMutex); + /* BUG: this leaks memory */ + if (!TerminateThread(hThread, 0)) { + /* terminate fails, just give thread a chance to exit */ + if (InterlockedExchange(&pipeTI->state, + PTI_STATE_STOP) != PTI_STATE_DOWN) { + pipeTI = NULL; + } + }; + Tcl_MutexUnlock(&pipeMutex); + } + } + } + } + + *pipeTIPtr = NULL; + if (pipeTI) { + CloseHandle(pipeTI->evControl); + #ifndef _PTI_USE_CKALLOC + free(pipeTI); + #else + ckfree(pipeTI); + #endif + } +} + +void +PipeThreadExit( + PipeThreadInfo **pipeTIPtr) +{ + LONG state; + PipeThreadInfo *pipeTI = *pipeTIPtr; + /* + * If state of thread was set to stop (exactly), we can sane free its info + * structure, otherwise it is shared with main thread, so main thread will + * own it. + */ + if (!pipeTI) { + return; + } + *pipeTIPtr = NULL; + if ((state = InterlockedExchange(&pipeTI->state, + PTI_STATE_DOWN)) == PTI_STATE_STOP) { + CloseHandle(pipeTI->evControl); + #ifndef _PTI_USE_CKALLOC + free(pipeTI); + #else + ckfree(pipeTI); + /* be sure all subsystems used are finalized */ + Tcl_FinalizeThread(); + #endif + } +} typedef struct PipeInfo { struct PipeInfo *nextPtr; /* Pointer to next registered pipe. */ Tcl_Channel channel; /* Pointer to channel structure. */ int validMask; /* OR'ed combination of TCL_READABLE, @@ -107,28 +426,21 @@ int numPids; /* Number of processes attached to pipe. */ Tcl_Pid *pidPtr; /* Pids of attached processes. */ Tcl_ThreadId threadId; /* Thread to which events should be reported. * This value is used by the reader/writer * threads. */ + PipeThreadInfo *writeTI; /* Thread info of writer and reader, this */ + PipeThreadInfo *readTI; /* structure owned by corresponding thread. */ HANDLE writeThread; /* Handle to writer thread. */ HANDLE readThread; /* Handle to reader thread. */ + HANDLE writable; /* Manual-reset event to signal when the * writer thread has finished waiting for the * current buffer to be written. */ HANDLE readable; /* Manual-reset event to signal when the * reader thread has finished waiting for * input. */ - HANDLE startWriter; /* Auto-reset event used by the main thread to - * signal when the writer thread should - * attempt to write to the pipe. */ - 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. */ - 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 * write. Set to 0 if no error has been * detected. This word is shared with the * writer thread so access must be * synchronized with the writable object. @@ -1597,31 +1909,31 @@ /* * Start the background reader thread. */ infoPtr->readable = CreateEvent(NULL, TRUE, TRUE, NULL); - infoPtr->startReader = CreateEvent(NULL, FALSE, FALSE, NULL); - infoPtr->stopReader = CreateEvent(NULL, TRUE, FALSE, NULL); infoPtr->readThread = CreateThread(NULL, 256, PipeReaderThread, - infoPtr, 0, &id); + PipeThreadCreateTI(&infoPtr->readTI, infoPtr), 0, &id); SetThreadPriority(infoPtr->readThread, THREAD_PRIORITY_HIGHEST); infoPtr->validMask |= TCL_READABLE; } else { + infoPtr->readTI = NULL; infoPtr->readThread = 0; } if (writeFile != NULL) { /* * Start the background writer thread. */ infoPtr->writable = CreateEvent(NULL, TRUE, TRUE, NULL); - infoPtr->startWriter = CreateEvent(NULL, FALSE, FALSE, NULL); - infoPtr->stopWriter = CreateEvent(NULL, TRUE, FALSE, NULL); infoPtr->writeThread = CreateThread(NULL, 256, PipeWriterThread, - infoPtr, 0, &id); - SetThreadPriority(infoPtr->readThread, THREAD_PRIORITY_HIGHEST); + PipeThreadCreateTI(&infoPtr->writeTI, infoPtr), 0, &id); + SetThreadPriority(infoPtr->writeThread, THREAD_PRIORITY_HIGHEST); infoPtr->validMask |= TCL_WRITABLE; + } else { + infoPtr->writeTI = NULL; + infoPtr->writeThread = 0; } /* * For backward compatibility with previous versions of Tcl, we use * "file%d" as the base name for pipes even though it would be more @@ -1803,11 +2115,10 @@ PipeInfo *pipePtr = (PipeInfo *) instanceData; Tcl_Channel errChan; int errorCode, result; PipeInfo *infoPtr, **nextPtrPtr; ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey); - DWORD exitCode; errorCode = 0; result = 0; if ((!flags || flags == TCL_CLOSE_READ) && (pipePtr->readFile != NULL)) { @@ -1816,59 +2127,14 @@ * done before we can close the file, since the thread may be blocking * trying to read from the pipe. */ if (pipePtr->readThread) { - /* - * The thread may already have closed on its own. Check its exit - * code. - */ - - GetExitCodeThread(pipePtr->readThread, &exitCode); - - if (exitCode == STILL_ACTIVE) { - /* - * Set the stop event so that if the reader thread is blocked - * in PipeReaderThread on WaitForMultipleEvents, it will exit - * cleanly. - */ - - SetEvent(pipePtr->stopReader); - - /* - * Wait at most 20 milliseconds for the reader thread to - * close. - */ - - if (WaitForSingleObject(pipePtr->readThread, - 20) == WAIT_TIMEOUT) { - /* - * The thread must be blocked waiting for the pipe to - * become readable in ReadFile(). There isn't a clean way - * to exit the thread from this condition. We should - * terminate the child process instead to get the reader - * thread to fall out of ReadFile with a FALSE. (below) is - * not the correct way to do this, but will stay here - * until a better solution is found. - * - * 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. - */ - - Tcl_MutexLock(&pipeMutex); - - /* BUG: this leaks memory */ - TerminateThread(pipePtr->readThread, 0); - Tcl_MutexUnlock(&pipeMutex); - } - } - + + PipeThreadStop(&pipePtr->readTI, pipePtr->readThread); CloseHandle(pipePtr->readThread); CloseHandle(pipePtr->readable); - CloseHandle(pipePtr->startReader); - CloseHandle(pipePtr->stopReader); pipePtr->readThread = NULL; } if (TclpCloseFile(pipePtr->readFile) != 0) { errorCode = errno; } @@ -1876,81 +2142,36 @@ pipePtr->readFile = NULL; } if ((!flags || flags & TCL_CLOSE_WRITE) && (pipePtr->writeFile != NULL)) { if (pipePtr->writeThread) { + /* * Wait for the writer thread to finish the current buffer, then * terminate the thread and close the handles. If the channel is - * nonblocking but blocked during exit, bail out since the worker + * nonblocking or may block during exit, bail out since the worker * thread is not interruptible and we want TIP#398-fast-exit. */ - if (TclInExit() - && (pipePtr->flags & PIPE_ASYNC)) { + if ((pipePtr->flags & PIPE_ASYNC) && TclInExit()) { /* give it a chance to leave honorably */ - SetEvent(pipePtr->stopWriter); + PipeThreadStopSignal(&pipePtr->writeTI); - if (WaitForSingleObject(pipePtr->writable, 0) == WAIT_TIMEOUT) { + if (WaitForSingleObject(pipePtr->writable, 20) == WAIT_TIMEOUT) { return EWOULDBLOCK; } } else { WaitForSingleObject(pipePtr->writable, INFINITE); } - /* - * The thread may already have closed on it's own. Check its exit - * code. - */ - - GetExitCodeThread(pipePtr->writeThread, &exitCode); - - if (exitCode == STILL_ACTIVE) { - /* - * Set the stop event so that if the reader thread is blocked - * in PipeReaderThread on WaitForMultipleEvents, it will exit - * cleanly. - */ - - SetEvent(pipePtr->stopWriter); - - /* - * Wait at most 20 milliseconds for the reader thread to - * close. - */ - - if (WaitForSingleObject(pipePtr->writeThread, - 20) == WAIT_TIMEOUT) { - /* - * The thread must be blocked waiting for the pipe to - * consume input in WriteFile(). There isn't a clean way - * to exit the thread from this condition. We should - * terminate the child process instead to get the writer - * thread to fall out of WriteFile with a FALSE. (below) - * is not the correct way to do this, but will stay here - * until a better solution is found. - * - * 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. - */ - - Tcl_MutexLock(&pipeMutex); - - /* BUG: this leaks memory */ - TerminateThread(pipePtr->writeThread, 0); - Tcl_MutexUnlock(&pipeMutex); - } - } - - CloseHandle(pipePtr->writeThread); + PipeThreadStop(&pipePtr->writeTI, pipePtr->writeThread); + CloseHandle(pipePtr->writable); - CloseHandle(pipePtr->startWriter); - CloseHandle(pipePtr->stopWriter); + CloseHandle(pipePtr->writeThread); pipePtr->writeThread = NULL; } if (TclpCloseFile(pipePtr->writeFile) != 0) { if (errorCode == 0) { errorCode = errno; @@ -2200,11 +2421,11 @@ infoPtr->writeBuf = ckalloc(toWrite); } memcpy(infoPtr->writeBuf, buf, (size_t) toWrite); infoPtr->toWrite = toWrite; ResetEvent(infoPtr->writable); - SetEvent(infoPtr->startWriter); + PipeThreadSignal(&infoPtr->writeTI); bytesWritten = toWrite; } else { /* * In the blocking case, just try to write the buffer directly. This * avoids an unnecessary copy. @@ -2784,11 +3005,11 @@ /* * There wasn't any data available, so reset the thread and try again. */ ResetEvent(infoPtr->readable); - SetEvent(infoPtr->startReader); + PipeThreadSignal(&infoPtr->readTI); } } /* *---------------------------------------------------------------------- @@ -2812,36 +3033,30 @@ static DWORD WINAPI PipeReaderThread( LPVOID arg) { - PipeInfo *infoPtr = (PipeInfo *)arg; - HANDLE *handle = ((WinFile *) infoPtr->readFile)->handle; + PipeThreadInfo *pipeTI = (PipeThreadInfo *)arg; + PipeInfo *infoPtr = NULL; /* access info only after success init/wait */ + HANDLE handle = NULL; DWORD count, err; int done = 0; - HANDLE wEvents[2]; - DWORD waitResult; - - wEvents[0] = infoPtr->stopReader; - wEvents[1] = infoPtr->startReader; - + while (!done) { /* * Wait for the main thread to signal before attempting to wait on the * pipe becoming readable. */ - - 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 (!PipeThreadWaitForSignal(&pipeTI)) { + /* exit */ break; } + + if (!infoPtr) { + infoPtr = (PipeInfo *)pipeTI->clientData; + handle = ((WinFile *) infoPtr->readFile)->handle; + } /* * Try waiting for 0 bytes. This will block until some data is * available on NT, but will return immediately on Win 95. So, if no * data is available after the first read, we block until we can read @@ -2885,11 +3100,10 @@ break; } } } - /* * Signal the main thread by signalling the readable event and then * waking up the notifier thread. */ @@ -2911,10 +3125,16 @@ Tcl_ThreadAlert(infoPtr->threadId); } Tcl_MutexUnlock(&pipeMutex); } + /* + * If state of thread was set to stop, we can sane free info structure, + * otherwise it is shared with main thread, so main thread will own it + */ + PipeThreadExit(&pipeTI); + return 0; } /* *---------------------------------------------------------------------- @@ -2935,39 +3155,33 @@ static DWORD WINAPI PipeWriterThread( LPVOID arg) { - PipeInfo *infoPtr = (PipeInfo *)arg; - HANDLE *handle = ((WinFile *) infoPtr->writeFile)->handle; + PipeThreadInfo *pipeTI = (PipeThreadInfo *)arg; + PipeInfo *infoPtr = NULL; /* access info only after success init/wait */ + HANDLE handle = NULL, writable = NULL; DWORD count, toWrite; char *buf; int done = 0; - HANDLE wEvents[2]; - DWORD waitResult; - - wEvents[0] = infoPtr->stopWriter; - wEvents[1] = infoPtr->startWriter; while (!done) { /* * 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 (waitResult == WAIT_OBJECT_0) { - SetEvent(infoPtr->writable); - } - - break; + if (!PipeThreadWaitForSignal(&pipeTI)) { + /* exit */ + if (writable) { + SetEvent(writable); + } + break; + } + + if (!infoPtr) { + infoPtr = (PipeInfo *)pipeTI->clientData; + handle = ((WinFile *) infoPtr->writeFile)->handle; + writable = infoPtr->writable; } buf = infoPtr->writeBuf; toWrite = infoPtr->toWrite; @@ -2989,11 +3203,11 @@ /* * Signal the main thread by signalling the writable event and then * waking up the notifier thread. */ - SetEvent(infoPtr->writable); + SetEvent(writable); /* * Alert the foreground thread. Note that we need to treat this like a * critical section so the foreground thread does not terminate this * thread while we are holding a mutex in the notifier code. @@ -3009,10 +3223,16 @@ Tcl_ThreadAlert(infoPtr->threadId); } Tcl_MutexUnlock(&pipeMutex); } + /* + * If state of thread was set to stop, we can sane free info structure, + * otherwise it is shared with main thread, so main thread will own it. + */ + PipeThreadExit(&pipeTI); + return 0; } /* *---------------------------------------------------------------------- Index: win/tclWinSerial.c ================================================================== --- win/tclWinSerial.c +++ win/tclWinSerial.c @@ -92,17 +92,19 @@ * This value is used by the reader/writer * threads. */ OVERLAPPED osRead; /* OVERLAPPED structure for read operations. */ OVERLAPPED osWrite; /* OVERLAPPED structure for write operations */ 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. */ + * 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 @@ -641,18 +643,33 @@ /* * 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. */ - Tcl_MutexLock(&serialMutex); - - /* BUG: this leaks memory */ - TerminateThread(serialPtr->writeThread, 0); - - Tcl_MutexUnlock(&serialMutex); + 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); @@ -1317,10 +1334,15 @@ DWORD bytesWritten, toWrite, waitResult; 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; @@ -1402,10 +1424,16 @@ Tcl_ThreadAlert(infoPtr->threadId); } 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; + return 0; } /* *---------------------------------------------------------------------- @@ -1529,12 +1557,17 @@ 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); } /* * Files have default translation of AUTO and ^Z eof char, which means * that a ^Z will be accepted as EOF when reading.