Finishing touches. Remove obsolete code

1 files changed, 177 insertions, 224 deletions

diff --git a/win/tclWinConsole.c b/win/tclWinConsole.c
index 14cc6e5..08e7e56 100644
--- a/win/tclWinConsole.c
+++ b/win/tclWinConsole.c
@@ -10,7 +10,6 @@
  * this file, and for a DISCLAIMER OF ALL WARRANTIES.
  */
 
-#define TCL_CONSOLE_DEBUG
 #ifdef TCL_CONSOLE_DEBUG
 #undef NDEBUG /* Enable asserts */
 #endif
@@ -55,7 +54,14 @@
 
 static int initialized = 0;
 
-#define CONSOLE_BUFFER_SIZE 8000 // TODO - must be at least 2 :-)
+#ifdef TCL_CONSOLE_DEBUG
+#ifndef CONSOLE_BUFFER_SIZE
+/*  Force tiny to stress synchronization. Must be at least sizeof(WCHAR) :-) */
+#define CONSOLE_BUFFER_SIZE 10
+#endif
+#else
+#define CONSOLE_BUFFER_SIZE 4000 /* In bytes */
+#endif
 
 /*
  * Ring buffer for storing data. Actual data is from bufPtr[start]:bufPtr[size-1]
@@ -227,9 +233,7 @@ static ConsoleHandleInfo *AllocateConsoleHandleInfo(HANDLE consoleHandle,
 static ConsoleHandleInfo *FindConsoleInfo(const ConsoleChannelInfo *);
 static DWORD WINAPI	ConsoleReaderThread(LPVOID arg);
 static DWORD WINAPI	ConsoleWriterThread(LPVOID arg);
-#ifdef OBSOLETE
-static int	WaitForRead(ConsoleChannelInfo *infoPtr, int blocking);
-#endif
+static void		NudgeWatchers(HANDLE consoleHandle);
 
 /*
  * Static data.
@@ -251,10 +255,6 @@ static Tcl_ThreadDataKey dataKey;
  * likely not only complicate implementation but be slower due to multiple
  * locks being held. Note console channels also differ from other Tcl
  * channel types in that the channel<->OS descriptor mapping is not one-to-one.
- *
- * The gConsoleLock locks around access to the initialized variable, and it
- * is used to protect background threads from being terminated while they
- * are using APIs that hold locks. TBD - is this still true?
  */
 SRWLOCK gConsoleLock;
 
@@ -374,7 +374,6 @@ RingBufferIn(
     )
 {
     RingSizeT freeSpace;
-    RingSizeT endSpace;
 
     RINGBUFFER_ASSERT(ringPtr);
 
@@ -387,23 +386,25 @@ RingBufferIn(
 	srcLen = freeSpace;
     }
 
-    /* Copy as much as possible to the tail */
     if (ringPtr->capacity - ringPtr->start > ringPtr->length) {
 	/* There is room at the back */
 	RingSizeT endSpaceStart = ringPtr->start + ringPtr->length;
-	endSpace = ringPtr->capacity - endSpaceStart;
-	if (endSpace > srcLen) {
-	    endSpace = srcLen;
+	RingSizeT endSpace      = ringPtr->capacity - endSpaceStart;
+	if (endSpace >= srcLen) {
+	    /* Everything fits at the back */
+	    memmove(endSpaceStart + ringPtr->bufPtr, srcPtr, srcLen);
+	}
+	else {
+	    /* srcLen > endSpace */
+	    memmove(endSpaceStart + ringPtr->bufPtr, srcPtr, endSpace);
+	    memmove(ringPtr->bufPtr, endSpace + srcPtr, srcLen - endSpace);
 	}
-	memmove(endSpaceStart + ringPtr->bufPtr, srcPtr, endSpace);
     }
     else {
-	endSpace = 0;
-    }
-
-    /* Wrap around any left over data. Have already copied endSpace bytes */
-    if (srcLen > endSpace) {
-	memmove(ringPtr->bufPtr, endSpace + srcPtr, srcLen - endSpace);
+	/* No room at the back. Existing data wrap to front. */
+	RingSizeT wrapLen =
+	    ringPtr->start + ringPtr->length - ringPtr->capacity;
+	memmove(wrapLen + ringPtr->bufPtr, srcPtr, srcLen);
     }
 
     ringPtr->length += srcLen;
@@ -716,6 +717,43 @@ ProcExitHandler(
 }
 
 /*
+ *------------------------------------------------------------------------
+ *
+ * NudgeWatchers --
+ *
+ *    Wakes up all threads which have file event watchers on the passed
+ *    console handle.
+ *
+ *    The function locks and releases gConsoleLock.
+ *    Caller must not be holding locks that will violate lock hierarchy.
+ *
+ * Results:
+ *    None.
+ *
+ * Side effects:
+ *    As above.
+ *------------------------------------------------------------------------
+ */
+void NudgeWatchers (HANDLE consoleHandle)
+{
+    ConsoleChannelInfo *chanInfoPtr;
+    AcquireSRWLockShared(&gConsoleLock); /* Shared-read lock */
+    for (chanInfoPtr = gWatchingChannelList; chanInfoPtr;
+	 chanInfoPtr = chanInfoPtr->nextWatchingChannelPtr) {
+	/*
+	 * Notify channels interested in our handle AND that have
+	 * a thread attached.
+	 * No lock needed for chanInfoPtr. See ConsoleChannelInfo.
+	 */
+	if (chanInfoPtr->handle == consoleHandle
+	    && chanInfoPtr->threadId != NULL) {
+	    Tcl_ThreadAlert(chanInfoPtr->threadId);
+	}
+    }
+    ReleaseSRWLockShared(&gConsoleLock);
+}
+
+/*
  *----------------------------------------------------------------------
  *
  * ConsoleSetupProc --
@@ -760,10 +798,12 @@ ConsoleSetupProc(
 	if (handleInfoPtr != NULL) {
 	    AcquireSRWLockShared(&handleInfoPtr->lock);
 	    if ((chanInfoPtr->watchMask & TCL_READABLE)
-		&& RingBufferLength(&handleInfoPtr->buffer) > 0) {
+		&& (RingBufferLength(&handleInfoPtr->buffer) > 0
+		    || handleInfoPtr->lastError != ERROR_SUCCESS)) {
 		block = 0; /* Input data available */
 	    }
 	    else if (RingBufferFreeSpace(&handleInfoPtr->buffer) > 0) {
+		/* TCL_WRITABLE */
 		block = 0; /* Output space available */
 	    }
 	    ReleaseSRWLockShared(&handleInfoPtr->lock);
@@ -837,8 +877,9 @@ ConsoleCheckProc(
 	if (handleInfoPtr != NULL) {
 	    AcquireSRWLockShared(&handleInfoPtr->lock);
 	    if ((chanInfoPtr->watchMask & TCL_READABLE)
-		&& RingBufferLength(&handleInfoPtr->buffer) > 0) {
-		needEvent = 1; /* Input data available */
+		&& (RingBufferLength(&handleInfoPtr->buffer) > 0
+		    || handleInfoPtr->lastError != ERROR_SUCCESS)) {
+		needEvent = 1; /* Input data available or error/EOF */
 	    }
 	    else if (RingBufferFreeSpace(&handleInfoPtr->buffer) > 0) {
 		needEvent = 1; /* Output space available */
@@ -974,7 +1015,7 @@ ConsoleCloseProc(
 	 * Don't close the Win32 handle if the handle is a standard channel
 	 * during the thread exit process. Otherwise, one thread may kill the
 	 * stdio of another. TODO - an explicit close in script will still close
-	 * it.
+	 * it. Is that desired behavior?
 	 */
 	if (!TclInThreadExit()
 	    || ((GetStdHandle(STD_INPUT_HANDLE) != chanInfoPtr->handle)
@@ -1050,8 +1091,8 @@ ConsoleInputProc(
 	/*
 	 * No data available.
 	 *  - If an error was recorded, generate that and reset it.
-	 *  - If EOF, indicate as much. TODO - can console thread still be
-	 * running in that case?
+	 *  - If EOF, indicate as much. It is up to application to close
+	 *    the channel.
 	 *  - Otherwise, if non-blocking return EAGAIN or wait for more data.
 	 */
 	if (handleInfoPtr->lastError != 0) {
@@ -1374,112 +1415,6 @@ ConsoleGetHandleProc(
     return TCL_OK;
 }
 
-#ifdef OBSOLETE
-/*
- *----------------------------------------------------------------------
- *
- * WaitForRead --
- *
- *	Wait until some data is available, the console is at EOF or the reader
- *	thread is blocked waiting for data (if the channel is in non-blocking
- *	mode).
- *
- * Results:
- *	Returns 1 if console is readable. Returns 0 if there is no data on the
- *	console, but there is buffered data. Returns -1 if an error occurred.
- *	If an error occurred, the threads may not be synchronized.
- *
- * Side effects:
- *	Updates the shared state flags. If no error occurred, the reader
- *	thread is blocked waiting for a signal from the main thread.
- *
- *----------------------------------------------------------------------
- */
-
-static int
-WaitForRead(
-    ConsoleChannelInfo *infoPtr,	/* Console state. */
-    int blocking)		/* Indicates whether call should be blocking
-				 * or not. */
-{
-    DWORD timeout, count;
-    HANDLE *handle = (HANDLE *)infoPtr->handle;
-    ConsoleThreadInfo *threadInfo = &infoPtr->reader;
-    INPUT_RECORD input;
-
-    while (1) {
-	/*
-	 * Synchronize with the reader thread.
-	 */
-
-	/* avoid blocking if pipe-thread exited */
-	timeout = (!blocking || !TclPipeThreadIsAlive(&threadInfo->TI)
-		|| TclInExit() || TclInThreadExit()) ? 0 : INFINITE;
-	if (WaitForSingleObject(threadInfo->readyEvent, timeout) == WAIT_TIMEOUT) {
-	    /*
-	     * The reader thread is blocked waiting for data and the channel
-	     * is in non-blocking mode.
-	     */
-
-	    errno = EWOULDBLOCK;
-	    return -1;
-	}
-
-	/*
-	 * At this point, the two threads are synchronized, so it is safe to
-	 * access shared state.
-	 */
-
-	/*
-	 * If the console has hit EOF, it is always readable.
-	 */
-
-	if (infoPtr->readFlags & CONSOLE_EOF) {
-	    return 1;
-	}
-
-	if (PeekConsoleInputW(handle, &input, 1, &count) == FALSE) {
-	    /*
-	     * Check to see if the peek failed because of EOF.
-	     */
-
-	    Tcl_WinConvertError(GetLastError());
-
-	    if (errno == EOF) {
-		infoPtr->readFlags |= CONSOLE_EOF;
-		return 1;
-	    }
-
-	    /*
-	     * Ignore errors if there is data in the buffer.
-	     */
-
-	    if (infoPtr->readFlags & CONSOLE_BUFFERED) {
-		return 0;
-	    } else {
-		return -1;
-	    }
-	}
-
-	/*
-	 * If there is data in the buffer, the console must be readable (since
-	 * it is a line-oriented device).
-	 */
-
-	if (infoPtr->readFlags & CONSOLE_BUFFERED) {
-	    return 1;
-	}
-
-	/*
-	 * There wasn't any data available, so reset the thread and try again.
-	 */
-
-	ResetEvent(threadInfo->readyEvent);
-	TclPipeThreadSignal(&threadInfo->TI);
-    }
-}
-#endif
-
 /*
  *----------------------------------------------------------------------
  *
@@ -1526,48 +1461,74 @@ ConsoleReaderThread(
     while (1) {
 
 	if (handleInfoPtr->numRefs == 1) {
-	    /* Sole reference. That's this thread. Exit since no one clients */
+	    /*
+	     * Sole reference. That's this thread. Exit since no clients
+	     * and no way for a thread to attach to a console after process
+	     * start.
+	     */
 	    break;
 	}
 
+	/*
+	 * Cases:
+	 * (1) The shared input buffer is full. Have to wait for an interp
+	 *     thread to read from it and make room.
+	 * (2) The shared input buffer has room and the thread private buffer
+	 *     has data. Copy into the shared input buffer.
+	 * (3) The channel has previously seen an error. Treat as EOF. Note
+	 *     this check is after the above so any data already available
+	 *     is passed on.
+	 * (4) Neither buffer has data and no errors. Go get some from console.
+	 *
+	 * There is some duplication of code below but easier to think about
+	 * rather than combining cases.
+	 */
 	if (RingBufferFreeSpace(&handleInfoPtr->buffer) == 0) {
-	    /* No room in buffer. Awaken any reader channels */
+	    /* Case (1) No room in buffer.*/
+
+	    /* Awaken any reader channels - TODO - is this really needed? */
 	    WakeConditionVariable(&handleInfoPtr->interpThreadCV);
-		/* XXX - does not wake up fileevent channels! */
 
 	    /* Release lock and wait for room */
 	    success = SleepConditionVariableSRW(&handleInfoPtr->consoleThreadCV,
 						&handleInfoPtr->lock,
 						INFINITE,
 						0);
-	    /* Note: lock has been acquired again! */
+	    /* Note: lock has been reacquired */
 
 	    if (!success && GetLastError() != ERROR_TIMEOUT) {
 		/* TODO - what can be done? Should not happen */
 		/* For now keep going */
 	    }
-	    continue; /* Restart loop so we can check for exit conditions */
-	}
-
-	/*
-	 * The shared buffer now has room. If we had any leftover from last
-	 * read, store that.
-	 */
-	if (inputLen > 0) {
-	    RingSizeT nStored;
+	} else if (inputLen > 0 || handleInfoPtr->lastError != 0) {
+	    /* Cases (2) and (3) - require notifications to interpreters */
 	    HANDLE consoleHandle;
-	    ConsoleChannelInfo *chanInfoPtr;
-
-	    nStored = RingBufferIn(&handleInfoPtr->buffer,
-				   inputOffset + inputChars,
-				   inputLen - inputOffset,
-				   1);
-	    inputOffset += nStored;
-	    if (inputOffset == inputLen) {
-		/* Temp buffer now empty */
-		inputOffset = 0;
-		inputLen = 0;
+	    if (inputLen > 0) {
+		/*
+		 * Case (2). Private buffer has data. Copy it over.
+		 */
+		RingSizeT nStored;
+
+		assert((inputLen - inputOffset) > 0);
+
+		nStored = RingBufferIn(&handleInfoPtr->buffer,
+				       inputOffset + inputChars,
+				       inputLen - inputOffset,
+				       1);
+		inputOffset += nStored;
+		if (inputOffset == inputLen) {
+		    /* Temp buffer now empty */
+		    inputOffset = 0;
+		    inputLen = 0;
+		}
 	    }
+	    else {
+		/*
+		 * Case (3). On error, nothing but inform caller and wait
+		 * We do not want to exit until there are no client interps.
+		 */
+	    }
+
 	    /* Wake up any threads waiting synchronously. */
 	    WakeConditionVariable(&handleInfoPtr->interpThreadCV);
 
@@ -1578,38 +1539,33 @@ ConsoleReaderThread(
 	     * relock it.
 	     */
 	    consoleHandle = handleInfoPtr->console;
+	    /*
+	     * Wake up all channels registered for file events. Note in
+	     * order to follow the locking hierarchy, we cannot hold any locks
+	     * when calling NudgeWatchers.
+	     */
 	    ReleaseSRWLockExclusive(&handleInfoPtr->lock);
-	    AcquireSRWLockShared(&gConsoleLock); /* Shared-read lock */
-	    for (chanInfoPtr = gWatchingChannelList; chanInfoPtr;
-		 chanInfoPtr = chanInfoPtr->nextWatchingChannelPtr) {
-		/*
-		 * Notify channels interested in our handle AND that have
-		 * a thread attached.
-		 * No lock needed for chanInfoPtr. See ConsoleChannelInfo.
-		 */
-		if (chanInfoPtr->handle == consoleHandle
-		    && chanInfoPtr->threadId != NULL) {
-		    Tcl_ThreadAlert(chanInfoPtr->threadId);
-		}
+	    NudgeWatchers(consoleHandle);
+
+	    AcquireSRWLockExclusive(&handleInfoPtr->lock);
+	}
+	else {
+	    /*
+	     * Case (4). Need to go get more data from console. We only
+	     * store the last error. It is up to channel handlers to decide
+	     * whether to close or what to do.
+	     */
+	    ReleaseSRWLockExclusive(&handleInfoPtr->lock);
+	    handleInfoPtr->lastError =
+		ReadConsoleChars(handleInfoPtr->console,
+				 (WCHAR *)inputChars,
+				 sizeof(inputChars) / sizeof(WCHAR),
+				 &inputLen);
+	    if (handleInfoPtr->lastError == 0) {
+		inputLen *= sizeof(WCHAR);
 	    }
-	    ReleaseSRWLockShared(&gConsoleLock);
 	    AcquireSRWLockExclusive(&handleInfoPtr->lock);
-	    continue; /* Restart loop */
 	}
-
-	/*
-	 * Need to go get more data from console. We only store the last
-	 * error. It is up to channel handlers to decide whether to close or
-	 * what to do.
-	 */
-	ReleaseSRWLockExclusive(&handleInfoPtr->lock);
-	handleInfoPtr->lastError =
-	    ReadConsoleChars(handleInfoPtr->console,
-			     (WCHAR *)inputChars,
-			     sizeof(inputChars) / sizeof(WCHAR),
-			     &inputLen);
-	inputLen *= sizeof(WCHAR);
-	AcquireSRWLockExclusive(&handleInfoPtr->lock);
     }
 
     /*
@@ -1617,6 +1573,8 @@ ConsoleReaderThread(
      * - remove the console from global list
      * - close the handle if still valid
      * - release the structure
+     * Note there is not need to check for any watchers because we only
+     * exit when there are no channels open to this console.
      */
     ReleaseSRWLockExclusive(&handleInfoPtr->lock);
     AcquireSRWLockExclusive(&gConsoleLock); /* Modifying - exclusive lock */
@@ -1632,14 +1590,15 @@ ConsoleReaderThread(
     /* No need for relocking - no other thread should have access to it now */
     RingBufferClear(&handleInfoPtr->buffer);
 
-    if (handleInfoPtr->console) {
+    if (handleInfoPtr->console
+	&& handleInfoPtr->lastError != ERROR_INVALID_HANDLE) {
 	SetConsoleMode(handleInfoPtr->console, handleInfoPtr->initMode);
+	/*
+	 * NOTE: we do not call CloseHandle(handleInfoPtr->console)
+	 * As per the GetStdHandle documentation, it need not be closed.
+	 * TODO - what about when application closes and re-opens? - Test
+	 */
     }
-    /*
-     * NOTE: we do not call CloseHandle(handleInfoPtr->console)
-     * As per the GetStdHandle documentation, it need not be closed.
-     * TODO - what about when application closes and re-opens? - Test
-     */
 
     ckfree(handleInfoPtr);
 
@@ -1669,8 +1628,13 @@ ConsoleWriterThread(LPVOID arg)
     ConsoleHandleInfo **iterator;
     ConsoleChannelInfo *chanInfoPtr = NULL;
     BOOL success;
-    char buffer[4000];
-    RingSizeT length;
+    RingSizeT numBytes;
+    /*
+     * This buffer size has no relation really with the size of the shared
+     * buffer. Could be bigger or smaller. Make larger as multiple threads
+     * could potentially be writing to it.
+     */
+    char buffer[2*CONSOLE_BUFFER_SIZE];
 
     /*
      * Keep looping until one of the following happens.
@@ -1698,9 +1662,9 @@ ConsoleWriterThread(LPVOID arg)
 	 * WCHAR's, i.e. even number of chars so do some length checks up
 	 * front.
 	 */
-	length = RingBufferLength(&handleInfoPtr->buffer);
-	length &= ~1; /* Copy integral number of WCHARs -> even number of bytes */
-	if (length == 0) {
+	numBytes = RingBufferLength(&handleInfoPtr->buffer);
+	numBytes &= ~1; /* Copy integral number of WCHARs -> even number of bytes */
+	if (numBytes == 0) {
 	    /* No data to write */
 	    if (handleInfoPtr->numRefs == 1) {
 		/*
@@ -1724,20 +1688,23 @@ ConsoleWriterThread(LPVOID arg)
 	}
 
 	/* We have data to write */
-	if (length > (sizeof(buffer) / sizeof(buffer[0]))) {
-	    length = sizeof(buffer);
+	if (numBytes > (sizeof(buffer) / sizeof(buffer[0]))) {
+	    numBytes = sizeof(buffer);
 	}
 	/* No need to check result, we already checked length bytes available */
-	RingBufferOut(&handleInfoPtr->buffer, buffer, length, 0);
+	RingBufferOut(&handleInfoPtr->buffer, buffer, numBytes, 0);
 
+	consoleHandle = handleInfoPtr->console;
 	WakeConditionVariable(&handleInfoPtr->interpThreadCV);
 	ReleaseSRWLockExclusive(&handleInfoPtr->lock);
 	offset = 0;
-	while (length > 0) {
-	    RingSizeT numWChars = length / sizeof(WCHAR);
+	while (numBytes > 0) {
+	    RingSizeT numWChars = numBytes / sizeof(WCHAR);
 	    DWORD status;
-	    status = WriteConsoleChars(
-		handleInfoPtr->console, (WCHAR *) (offset + buffer) , numWChars, &numWChars);
+	    status = WriteConsoleChars(handleInfoPtr->console,
+				       (WCHAR *)(offset + buffer),
+				       numWChars,
+				       &numWChars);
 	    if (status != 0) {
 		/* Only overwrite if no previous error */
 		if (handleInfoPtr->lastError == 0) {
@@ -1749,33 +1716,19 @@ ConsoleWriterThread(LPVOID arg)
 		 */
 		break;
 	    }
-	    length -= numWChars * sizeof(WCHAR);
+	    numBytes -= numWChars * sizeof(WCHAR);
 	    offset += numWChars * sizeof(WCHAR);
 	}
 
+	/* Wake up any threads waiting synchronously. */
+	WakeConditionVariable(&handleInfoPtr->interpThreadCV);
 	/*
 	 * Wake up all channels registered for file events. Note in
-	 * order to follow the locking hierarchy, we need to release
-	 * handleInfoPtr->lock before acquiring gConsoleLock and
-	 * relock it.
+	 * order to follow the locking hierarchy, we cannot hold any locks
+	 * when calling NudgeWatchers.
 	 */
-	/* Wake up any threads waiting synchronously. */
-	WakeConditionVariable(&handleInfoPtr->interpThreadCV);
-	AcquireSRWLockShared(&gConsoleLock); /* Shared-read lock */
-	consoleHandle = handleInfoPtr->console;
-	for (chanInfoPtr = gWatchingChannelList; chanInfoPtr;
-	     chanInfoPtr = chanInfoPtr->nextWatchingChannelPtr) {
-	    /*
-	     * Notify channels interested in our handle AND that have
-	     * a thread attached.
-	     * No lock needed for chanInfoPtr. See ConsoleChannelInfo.
-	     */
-	    if (chanInfoPtr->handle == consoleHandle
-		&& chanInfoPtr->threadId != NULL) {
-		Tcl_ThreadAlert(chanInfoPtr->threadId);
-	    }
-	}
-	ReleaseSRWLockShared(&gConsoleLock);
+	NudgeWatchers(consoleHandle);
+
 	AcquireSRWLockExclusive(&handleInfoPtr->lock);
     }