Skip to content

Commit

Permalink
Fix [bda99f2393]. Windows non-blocking gets stdin truncation. See tic…
Browse files Browse the repository at this point in the history 
…ket for resolution.
  • Loading branch information
@apnadkarni
apnadkarni committed Feb 26, 2024
2 parents e9c8634 + 3bcc2a3 commit eb1eb32
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 28 deletions.
8 changes: 4 additions & 4 deletions tests/winConsole.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]
Expand All @@ -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
Expand All @@ -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]
Expand Down
74 changes: 50 additions & 24 deletions win/tclWinConsole.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

/*
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -1666,7 +1684,6 @@ ConsoleReaderThread(
Tcl_Size nStored;

assert((inputLen - inputOffset) > 0);

nStored = RingBufferIn(&handleInfoPtr->buffer,
inputOffset + inputChars,
inputLen - inputOffset,
Expand Down Expand Up @@ -1713,47 +1730,56 @@ 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),
&inputLen);
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
* 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,
Expand Down

0 comments on commit eb1eb32

Please sign in to comment.