From 063aecb05d90ef87d7178fe8dd5d8948052156e8 Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Sun, 10 Jul 2022 10:21:06 +0000 Subject: Bypass reader thread for blocking reads. --- tests/winConsole.test | 20 ++++---- win/tclWinConsole.c | 129 +++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 115 insertions(+), 34 deletions(-) diff --git a/tests/winConsole.test b/tests/winConsole.test index cb1babc..d008d5c 100644 --- a/tests/winConsole.test +++ b/tests/winConsole.test @@ -45,13 +45,13 @@ proc prompt {prompt} { # Input tests -test console-gets-1.0 {Console blocking gets} -constraints {win interactive} -body { +test console-input-1.0 {Console blocking gets} -constraints {win interactive} -body { prompt "Type \"xyz\" and hit Enter: " gets stdin } -result xyz -test console-gets-1.1 {Console file channel: non-blocking gets} -constraints { - win interactive tbd +test console-input-1.1 {Console file channel: non-blocking gets} -constraints { + win interactive } -body { set oldmode [fconfigure stdin] @@ -72,25 +72,23 @@ test console-gets-1.1 {Console file channel: non-blocking gets} -constraints { #cleanup the fileevent fileevent stdin readable {} fconfigure stdin {*}$oldmode - puts [fconfigure stdin] set result } -result abc -test console-read-1.0 {Console blocking read} -constraints {win interactive} -setup { +test console-input-2.0 {Console blocking read} -constraints {win interactive} -setup { set oldmode [fconfigure stdin] fconfigure stdin -inputmode raw } -cleanup { fconfigure stdin {*}$oldmode } -body { - puts [fconfigure stdin] prompt "Type the key \"a\". Do NOT hit Enter. You will NOT see characters echoed." set c [read stdin 1] puts "" set c } -result a -test console-read-1.1 {Console file channel: non-blocking read} -constraints { +test console-input-2.1 {Console file channel: non-blocking read} -constraints { win interactive } -setup { set oldmode [fconfigure stdin] @@ -118,18 +116,18 @@ test console-read-1.1 {Console file channel: non-blocking read} -constraints { set result {} vwait result fileevent stdin readable {} - puts "" set result } -result abc # Output tests -test console-puts-1.0 {Console blocking puts stdout} -constraints {win interactive} -body { +test console-output-1.0 {Console blocking puts stdout} -constraints {win interactive} -body { + puts [fconfigure stdin] puts stdout "123" yesno "Did you see the string \"123\"?" } -result 1 -test console-puts-1.1 {Console non-blocking puts stdout} -constraints { +test console-output-1.1 {Console non-blocking puts stdout} -constraints { win interactive } -setup { set oldmode [fconfigure stdout] @@ -151,7 +149,7 @@ test console-puts-1.1 {Console non-blocking puts stdout} -constraints { yesno "Did you see 1, 2, 3 printed on consecutive lines?" } -result 1 -test console-puts-2.0 {Console blocking puts stderr} -constraints {win interactive} -body { +test console-output-2.0 {Console blocking puts stderr} -constraints {win interactive} -body { puts stderr "456" yesno "Did you see the string \"456\"?" } -result 1 diff --git a/win/tclWinConsole.c b/win/tclWinConsole.c index 2e60c91..2cc16d9 100644 --- a/win/tclWinConsole.c +++ b/win/tclWinConsole.c @@ -27,7 +27,7 @@ * corresponding to stdin, stdout, stderr) * * - Consoles are created / inherited at process startup. There is currently - * no way in Tcl to programmatically create a console. Even if there were + * no way in Tcl to programmatically create a console. Even if these were * added the above Windows limitation would still apply. * * - Unlike files, sockets etc. where there is a one-to-one @@ -37,8 +37,7 @@ * * - Even with multiple threads, more than one file event handler is unlikely. * It does not make sense for multiple threads to register handlers for - * stdin because the input would be randomly fragmented amongst the threads - * (not even on a per line basis). + * stdin because the input would be randomly fragmented amongst the threads. * * Various design factors are driven by the above, e.g. use of lists instead * of hash tables (at most 3 console handles) and use of global instead of @@ -48,14 +47,8 @@ * * Some additional design notes/reminders for the future: * - * All input is done through the reader thread, even synchronous reads of - * stdin which in theory could be done directly by the interpreter threads. - * This is because I'm not entirely confident about multithreaded access to - * the ReadConsole API (probably ok since Microsoft does not warn against - * this) and also the API requires reading an even number of bytes (WCHAR) - * while the channel callback has no such restriction (in theory). - * Accounting for that in the callbacks is doable but slightly tricky while - * straightforward in the reader thread because of its double buffering. + * Aligned, synchronous reads are done directly by interpreter thread. + * Unaligned or asynchronous reads are done through the reader thread. * * The reader thread does not read ahead. That is, it will not post a read * until some interpreter thread is actually requesting a read. This is @@ -1153,9 +1146,6 @@ ConsoleInputProc( chanInfoPtr->handle = INVALID_HANDLE_VALUE; break; } - /* Request console reader thread for data */ - handleInfoPtr->flags |= CONSOLE_DATA_AWAITED; - WakeConditionVariable(&handleInfoPtr->consoleThreadCV); /* For async, tell caller we are blocked */ if (chanInfoPtr->flags & CONSOLE_ASYNC) { @@ -1165,10 +1155,51 @@ 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 + * them here (which is a little trickier than it might sound.) + */ + if ((1 & (ptrdiff_t)bufPtr) == 0 /* aligned buffer */ + && bufSize > 1 /* Not single byte read */ + ) { + DWORD lastError; + RingSizeT numChars; + ReleaseSRWLockExclusive(&handleInfoPtr->lock); + lastError = ReadConsoleChars(chanInfoPtr->handle, + (WCHAR *)bufPtr, + bufSize / sizeof(WCHAR), + &numChars); + /* NOTE lock released so DON'T break. Return instead */ + if (lastError != ERROR_SUCCESS) { + Tcl_WinConvertError(lastError); + *errorCode = Tcl_GetErrno(); + return -1; + } + else if (numChars > 0) { + /* Successfully read something. */ + return numChars * sizeof(WCHAR); + } + else { + /* + * Ctrl-C/Ctrl-Brk interrupt. Loop around to retry. + * We have to reacquire the lock. No worried about handleInfoPtr + * having gone away since the channel holds a reference. + */ + AcquireSRWLockExclusive(&handleInfoPtr->lock); + continue; + } + } + /* + * Deferring blocking read to reader thread. * Release the lock and sleep. Note that because the channel * holds a reference count on handleInfoPtr, it will not * be deallocated while the lock is released. */ + handleInfoPtr->flags |= CONSOLE_DATA_AWAITED; + WakeConditionVariable(&handleInfoPtr->consoleThreadCV); if (!SleepConditionVariableSRW(&handleInfoPtr->interpThreadCV, &handleInfoPtr->lock, INFINITE, @@ -1178,10 +1209,11 @@ ConsoleInputProc( numRead = -1; break; } + /* Lock is reacquired, loop back to try again */ } + if (chanInfoPtr->flags & CONSOLE_ASYNC) { - /* Async channels always want read ahead */ handleInfoPtr->flags |= CONSOLE_DATA_AWAITED; WakeConditionVariable(&handleInfoPtr->consoleThreadCV); } @@ -1541,6 +1573,49 @@ ConsoleGetHandleProc( } /* + *------------------------------------------------------------------------ + * + * ConsoleDataAvailable -- + * + * Checks if there is data in the console input queue. + * + * Results: + * Returns 1 if the input queue has data, -1 on error else 0 if empty. + * + * Side effects: + * None. + * + *------------------------------------------------------------------------ + */ + static int + ConsoleDataAvailable (HANDLE consoleHandle) +{ + INPUT_RECORD input[5]; + DWORD count; + DWORD i; + + /* + * Need at least one keyboard event. + */ + if (PeekConsoleInputW( + consoleHandle, input, sizeof(input) / sizeof(input[0]), &count) + == FALSE) { + return -1; + } + for (i = 0; i < count; ++i) { + /* + * Event must be a keydown because a trailing LF keyup event is always + * present for line based input. + */ + if (input[i].EventType == KEY_EVENT + && input[i].Event.KeyEvent.bKeyDown) { + return 1; + } + } + return 0; +} + +/* *---------------------------------------------------------------------- * * ConsoleReaderThread -- @@ -1563,7 +1638,6 @@ ConsoleReaderThread( { ConsoleHandleInfo *handleInfoPtr = (ConsoleHandleInfo *) arg; ConsoleHandleInfo **iterator; - BOOL success; char inputChars[200]; /* Temporary buffer */ RingSizeT inputLen = 0; RingSizeT inputOffset = 0; @@ -1655,7 +1729,8 @@ ConsoleReaderThread( * for password input. So only do so if at least one interpreter has * requested data. */ - if (handleInfoPtr->flags & CONSOLE_DATA_AWAITED) { + if ((handleInfoPtr->flags & CONSOLE_DATA_AWAITED) + && ConsoleDataAvailable(handleInfoPtr->console)) { DWORD error; /* Do not hold the lock while blocked in console */ ReleaseSRWLockExclusive(&handleInfoPtr->lock); @@ -1682,11 +1757,20 @@ ConsoleReaderThread( } } else { - /* Wait until an interp thread asks for data. */ - success = SleepConditionVariableSRW(&handleInfoPtr->consoleThreadCV, - &handleInfoPtr->lock, - INFINITE, - 0); + /* + * Either no one was asking for data, or no data was available. + * In the former case, wait until someone wakes us asking for + * data. In the latter case, there is no alternative but to + * 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, + &handleInfoPtr->lock, + sleepTime, + 0); } /* Loop again to check for exit or wait for readers to wake us */ @@ -1884,7 +1968,6 @@ ConsoleWriterThread(LPVOID arg) RingBufferClear(&handleInfoPtr->buffer); - ckfree(handleInfoPtr); return 0; -- cgit v0.12