diff --git a/tests/winConsole.test b/tests/winConsole.test index 3597fe305fa..e87c8704e7f 100644 --- a/tests/winConsole.test +++ b/tests/winConsole.test @@ -155,7 +155,7 @@ test console-input-2.1 {Console file channel: non-blocking read} -constraints { } -result abc test console-input-3.0 {Console gets blocking - long lines bug-bda99f2393} -constraints { - win interactive bug-bda99f2393 + win interactive } -body { prompt "Try typing a line of at least 256 characters. Hit ENTER exactly once unless you don't see another prompt.\n" gets stdin line @@ -164,7 +164,7 @@ test console-input-3.0 {Console gets blocking - long lines bug-bda99f2393} -cons } -result {1 1 1} test console-input-3.1 {Console gets blocking, small channel buffer size - long lines bug-bda99f2393} -constraints { - win interactive bug-bda99f2393 + win interactive } -body { prompt "Try typing a line of at least 256 characters. Hit ENTER exactly once unless you don't see another prompt.\n" set bufSize [fconfigure stdin -buffersize] @@ -176,7 +176,7 @@ test console-input-3.1 {Console gets blocking, small channel buffer size - long } -result {1 1 1} test console-input-3.2 {Console gets nonblocking - long lines bug-bda99f2393} -constraints { - win interactive bug-bda99f2393 + win interactive } -body { prompt "Try typing a line of at least 256 characters. Hit ENTER exactly once unless you don't see another prompt.\n" fconfigure stdin -blocking 0 @@ -189,7 +189,7 @@ test console-input-3.2 {Console gets nonblocking - long lines bug-bda99f2393} -c } -result {1 1 1} test console-input-3.3 {Console gets nonblocking small channel buffer size - long lines bug-bda99f2393} -constraints { - win interactive bug-bda99f2393 + win interactive } -body { prompt "Try typing a line of at least 256 characters. Hit ENTER exactly once unless you don't see another prompt.\n" set bufSize [fconfigure stdin -buffersize] diff --git a/win/tclWinConsole.c b/win/tclWinConsole.c index eb81370578d..acd5851e53a 100644 --- a/win/tclWinConsole.c +++ b/win/tclWinConsole.c @@ -70,14 +70,23 @@ static int gInitialized = 0; /* - * Permit CONSOLE_BUFFER_SIZE to be defined on build command for stress test. - * + * INPUT_BUFFER_SIZE is size of buffer passed to ReadConsole in bytes. + * Note that ReadConsole will only allow reading of line lengths up to the + * max of 256 and buffer size passed to it. So dropping this below 512 + * means user can type at most 256 chars. + */ +#ifndef INPUT_BUFFER_SIZE +#define INPUT_BUFFER_SIZE 8192 /* In bytes, so 4096 chars */ +#endif + +/* + * CONSOLE_BUFFER_SIZE is size of storage used in ring buffers. * In theory, at least sizeof(WCHAR) but note the Tcl channel bug * https://core.tcl-lang.org/tcl/tktview/b3977d199b08e3979a8da970553d5209b3042e9c * will cause failures in test suite if close to max input line in the suite. */ #ifndef CONSOLE_BUFFER_SIZE -#define CONSOLE_BUFFER_SIZE 8000 /* In bytes */ +#define CONSOLE_BUFFER_SIZE 8192 /* In bytes */ #endif /* @@ -1143,15 +1152,22 @@ ConsoleInputProc( /* * Blocking read. Just get data from directly from console. There - * is a small complication in that we can only read even number - * of bytes (wide-character API) and the destination buffer should be - * WCHAR aligned. If either condition is not met, we defer to the - * reader thread which handles these case rather than dealing with + * is a small complication in that + * 1. The destination buffer should be WCHAR aligned. + * 2. We can only read even number of bytes (wide-character API). + * 3. Caller has large enough buffer (else length of line user can + * enter will be limited) + * If any condition is not met, we defer to the + * reader thread which handles these cases rather than dealing with * them here (which is a little trickier than it might sound.) + * + * TODO - not clear this block is a useful optimization. bufSize by + * default is 4K which is < INPUT_BUFFER_SIZE and will rarely be + * increased on stdin. */ if ((1 & (size_t)bufPtr) == 0 /* aligned buffer */ - && bufSize > 1 /* Not single byte read */ - ) { + && (1 & bufSize) == 0 /* Even number of bytes */ + && bufSize > INPUT_BUFFER_SIZE) { DWORD lastError; Tcl_Size numChars; ReleaseSRWLockExclusive(&handleInfoPtr->lock); @@ -1630,9 +1646,11 @@ ConsoleReaderThread( { ConsoleHandleInfo *handleInfoPtr = (ConsoleHandleInfo *) arg; ConsoleHandleInfo **iterator; - char inputChars[200]; /* Temporary buffer */ Tcl_Size inputLen = 0; Tcl_Size inputOffset = 0; + Tcl_Size lastReadSize = 0; + DWORD sleepTime; + char inputChars[INPUT_BUFFER_SIZE]; /* * Keep looping until one of the following happens. @@ -1666,7 +1684,6 @@ ConsoleReaderThread( Tcl_Size nStored; assert((inputLen - inputOffset) > 0); - nStored = RingBufferIn(&handleInfoPtr->buffer, inputOffset + inputChars, inputLen - inputOffset, @@ -1713,21 +1730,27 @@ ConsoleReaderThread( continue; } + assert(inputLen == 0); + /* - * Both shared buffer and private buffer are empty. Need to go get - * data from console but do not want to read ahead because the - * interp thread might change the read mode, e.g. turning off echo - * for password input. So only do so if at least one interpreter has - * requested data. + * Read more data in two cases: + * 1. The previous read filled the buffer and there could be more + * data in the console internal *text* buffer. Note + * ConsolePendingInput (checked in ConsoleDataAvailable) will NOT + * show this. It holds input events not yet translated to text. + * 2. Tcl threads want more data AND there is data in the + * ConsolePendingInput buffer. The latter check necessary because + * we do not want to read ahead because the interp thread might + * change the read mode, e.g. turning off echo for password + * input. So only do so if at least one interpreter has requested + * data. */ - if ((handleInfoPtr->flags & CONSOLE_DATA_AWAITED) - && ConsoleDataAvailable(handleInfoPtr->console)) { + if (lastReadSize == sizeof(inputChars) + || ((handleInfoPtr->flags & CONSOLE_DATA_AWAITED) + && ConsoleDataAvailable(handleInfoPtr->console))) { DWORD error; /* Do not hold the lock while blocked in console */ ReleaseSRWLockExclusive(&handleInfoPtr->lock); - /* - * Note - the temporary buffer serves two purposes. It - */ error = ReadConsoleChars(handleInfoPtr->console, (WCHAR *)inputChars, sizeof(inputChars) / sizeof(WCHAR), @@ -1735,17 +1758,21 @@ ConsoleReaderThread( AcquireSRWLockExclusive(&handleInfoPtr->lock); if (error == 0) { inputLen *= sizeof(WCHAR); - } else { + lastReadSize = inputLen; + } + else { /* * We only store the last error. It is up to channel * handlers whether to close or not in case of errors. */ + lastReadSize = 0; handleInfoPtr->lastError = error; if (handleInfoPtr->lastError == ERROR_INVALID_HANDLE) { handleInfoPtr->console = INVALID_HANDLE_VALUE; } } - } else { + } + else { /* * Either no one was asking for data, or no data was available. * In the former case, wait until someone wakes us asking for @@ -1753,7 +1780,6 @@ ConsoleReaderThread( * poll since ReadConsole does not support async operation. * So sleep for a short while and loop back to retry. */ - DWORD sleepTime; sleepTime = handleInfoPtr->flags & CONSOLE_DATA_AWAITED ? 50 : INFINITE; SleepConditionVariableSRW(&handleInfoPtr->consoleThreadCV,