[5adc350683] Stop Tcl forcing EOF condition on channels to be permanent.

It may be fleeting, and all parts of Tcl channel ecosystem have to deal
with that.  New assertions and tests to keep us on track.

4 files changed, 297 insertions, 52 deletions

diff --git a/generic/tclIO.c b/generic/tclIO.c
index 0122ec9..b3af1f5 100644
--- a/generic/tclIO.c
+++ b/generic/tclIO.c
@@ -3977,6 +3977,21 @@ Tcl_GetsObj(
     }
 
     /*
+     * If we're sitting ready to read the eofchar, there's no need to
+     * do it.
+     */
+
+    if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) {
+	SetFlag(statePtr, CHANNEL_EOF);
+	assert( statePtr->inputEncodingFlags & TCL_ENCODING_END );
+	assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) );
+
+	/* TODO: Do we need this? */
+	UpdateInterest(chanPtr);
+	return -1;
+    }
+
+    /*
      * A binary version of Tcl_GetsObj. This could also handle encodings that
      * are ascii-7 pure (iso8859, utf-8, ...) with a final encoding conversion
      * done on objPtr.
@@ -4194,6 +4209,7 @@ Tcl_GetsObj(
 	    dstEnd = eof;
 	    SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF);
 	    statePtr->inputEncodingFlags |= TCL_ENCODING_END;
+	    ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR);
 	}
 	if (GotFlag(statePtr, CHANNEL_EOF)) {
 	    skip = 0;
@@ -4307,6 +4323,13 @@ Tcl_GetsObj(
      */
 
   done:
+	assert(!GotFlag(statePtr, CHANNEL_EOF)
+		|| GotFlag(statePtr, CHANNEL_STICKY_EOF)
+		|| Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0);
+
+	assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED)
+		== (CHANNEL_EOF|CHANNEL_BLOCKED)) );
+
     /*
      * Regenerate the top channel, in case it was changed due to
      * self-modifying reflected transforms.
@@ -4330,6 +4353,11 @@ Tcl_GetsObj(
  *	end-of-line or end-of-file has been seen. Bytes read from the input
  *	channel return as a ByteArray obj.
  *
+ *	WARNING!  The notion of "binary" used here is different from
+ *	notions of "binary" used in other places.  In particular, this
+ *	"binary" routine may be called when an -eofchar is set on the
+ * 	channel.
+ *
  * Results:
  *	Number of characters accumulated in the object or -1 if error,
  *	blocked, or EOF. If -1, use Tcl_GetErrno() to retrieve the POSIX error
@@ -4414,6 +4442,17 @@ TclGetsObjBinary(
 	    if (bufPtr == NULL) {
 		goto restore;
 	    }
+	} else {
+	    /*
+	     * Incoming CHANNEL_STICKY_EOF is filtered out on entry.
+	     * A new CHANNEL_STICKY_EOF set in this routine leads to
+	     * return before coming back here.  When we are not dealing
+	     * with CHANNEL_STICKY_EOF, a CHANNEL_EOF implies an
+	     * empty buffer.  Here the buffer is non-empty so we know
+	     * we're a non-EOF */
+
+	    assert ( !GotFlag(statePtr, CHANNEL_STICKY_EOF) );
+	    assert ( !GotFlag(statePtr, CHANNEL_EOF) );
 	}
 
 	dst = (unsigned char *) RemovePoint(bufPtr);
@@ -4455,6 +4494,7 @@ TclGetsObjBinary(
 
 	    SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF);
 	    statePtr->inputEncodingFlags |= TCL_ENCODING_END;
+	    ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR);
 	}
 	if (GotFlag(statePtr, CHANNEL_EOF)) {
 	    skip = 0;
@@ -4564,6 +4604,11 @@ TclGetsObjBinary(
      */
 
   done:
+	assert(!GotFlag(statePtr, CHANNEL_EOF)
+		|| GotFlag(statePtr, CHANNEL_STICKY_EOF)
+		|| Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0);
+	assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED)
+		== (CHANNEL_EOF|CHANNEL_BLOCKED)) );
     UpdateInterest(chanPtr);
     TclChannelRelease((Tcl_Channel)chanPtr);
     return copiedTotal;
@@ -4694,6 +4739,17 @@ FilterInputBytes(
 	    gsPtr->rawRead = 0;
 	    return -1;
 	}
+    } else {
+	/*
+	 * Incoming CHANNEL_STICKY_EOF is filtered out on entry.
+	 * A new CHANNEL_STICKY_EOF set in this routine leads to
+	 * return before coming back here.  When we are not dealing
+	 * with CHANNEL_STICKY_EOF, a CHANNEL_EOF implies an
+	 * empty buffer.  Here the buffer is non-empty so we know
+	 * we're a non-EOF */
+
+	assert ( !GotFlag(statePtr, CHANNEL_STICKY_EOF) );
+	assert ( !GotFlag(statePtr, CHANNEL_EOF) );
     }
 
     /*
@@ -5018,6 +5074,7 @@ Tcl_ReadRaw(
 				/* State info for channel */
     int copied = 0;
 
+    assert(bytesToRead > 0);
     if (CheckChannelErrors(statePtr, TCL_READABLE | CHANNEL_RAW_MODE) != 0) {
 	return -1;
     }
@@ -5049,8 +5106,19 @@ Tcl_ReadRaw(
 	}
     }
 
-    /* Go to the driver if more data needed. */
+    /*
+     * Go to the driver only if we got nothing from pushback.
+     * Have to do it this way to avoid EOF mis-timings when we
+     * consider the ability that EOF may not be a permanent
+     * condition in the driver, and in that case we have to
+     * synchronize.
+     */
+
+    if (copied) {
+	return copied;
+    }
 
+    /* This test not needed. */
     if (bytesToRead > 0) {
 
 	int nread = ChanRead(chanPtr, readBuf, bytesToRead);
@@ -5073,12 +5141,10 @@ Tcl_ReadRaw(
 	    if (!GotFlag(statePtr, CHANNEL_BLOCKED) || copied == 0) {
 		copied = -1;
 	    }
-	} else if (copied > 0) {
+	} else {
 	    /*
-	     * nread == 0.  Driver is at EOF, but if copied>0 bytes
-	     * from pushback, then we should not signal it yet.
+	     * nread == 0.  Driver is at EOF. Let that state filter up.
 	     */
-	    ResetFlag(statePtr, CHANNEL_EOF);
 	}
     }
     return copied;
@@ -5177,19 +5243,11 @@ DoReadChars(
     ChannelState *statePtr = chanPtr->state;
 				/* State info for channel */
     ChannelBuffer *bufPtr;
-    int factor, copied, copiedNow, result;
-    Tcl_Encoding encoding;
+    int copied, copiedNow, result;
+    Tcl_Encoding encoding = statePtr->encoding;
     int binaryMode;
 #define UTF_EXPANSION_FACTOR	1024
-
-    /*
-     * This operation should occur at the top of a channel stack.
-     */
-
-    chanPtr = statePtr->topChanPtr;
-    encoding = statePtr->encoding;
-    factor = UTF_EXPANSION_FACTOR;
-    TclChannelPreserve((Tcl_Channel)chanPtr);
+    int factor = UTF_EXPANSION_FACTOR;
 
     binaryMode = (encoding == NULL)
 	    && (statePtr->inputTranslation == TCL_TRANSLATE_LF) 
@@ -5213,6 +5271,36 @@ DoReadChars(
 	}
     }
 
+    /*
+     * Early out when next read will see eofchar.
+     *
+     * NOTE: See DoRead for argument that it's a bug (one we're keeping)
+     * to have this escape before the one for zero-char read request.
+     */
+
+    if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) {
+	SetFlag(statePtr, CHANNEL_EOF);
+	assert( statePtr->inputEncodingFlags & TCL_ENCODING_END );
+	assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) );
+
+	UpdateInterest(chanPtr);
+	return 0;
+    }
+
+    /* Special handling for zero-char read request. */
+    if (toRead == 0) {
+	ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF);
+	UpdateInterest(chanPtr);
+	return 0;
+    }
+
+    /*
+     * This operation should occur at the top of a channel stack.
+     */
+
+    chanPtr = statePtr->topChanPtr;
+    TclChannelPreserve((Tcl_Channel)chanPtr);
+
     /* Must clear the BLOCKED flag here since we check before reading */
     ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF);
     for (copied = 0; (unsigned) toRead > 0; ) {
@@ -5290,6 +5378,11 @@ DoReadChars(
      * Update the notifier state so we don't block while there is still data
      * in the buffers.
      */
+	assert(!GotFlag(statePtr, CHANNEL_EOF)
+		|| GotFlag(statePtr, CHANNEL_STICKY_EOF)
+		|| Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0);
+	assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED)
+		== (CHANNEL_EOF|CHANNEL_BLOCKED)) );
     UpdateInterest(chanPtr);
     TclChannelRelease((Tcl_Channel)chanPtr);
     return copied;
@@ -5898,7 +5991,7 @@ TranslateInputEOL(
 
 	SetFlag(statePtr, CHANNEL_EOF | CHANNEL_STICKY_EOF);
 	statePtr->inputEncodingFlags |= TCL_ENCODING_END;
-	ResetFlag(statePtr, INPUT_SAW_CR);
+	ResetFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR);
     }
 }
 
@@ -6108,6 +6201,14 @@ GetInput(
     ChannelState *statePtr = chanPtr->state;
 				/* State info for channel */
 
+    /* 
+     * Verify that all callers know better than to call us when
+     * it's recorded that the next char waiting to be read is the
+     * eofchar.
+     */
+
+    assert( !GotFlag(statePtr, CHANNEL_STICKY_EOF) );
+
     /*
      * Prevent reading from a dead channel -- a channel that has been closed
      * but not yet deallocated, which can happen if the exit handler for
@@ -6119,18 +6220,24 @@ GetInput(
 	return EINVAL;
     }
 
-    /* 
-     * For a channel at EOF do not bother allocating buffers; there's
-     * nothing more to read.  Avoid calling the driver inputproc in
-     * case some of them do not react well to additional calls after
-     * they've reported an eof state..
-     * TODO: Candidate for a can't happen panic.
+    /*
+     * WARNING: There was once a comment here claiming that it was
+     * a bad idea to make another call to the inputproc of a channel
+     * driver when EOF has already been detected on the channel.  Through
+     * much of Tcl's history, this warning was then completely negated
+     * by having all (most?) read paths clear the EOF setting before
+     * reaching here.  So we had a guard that was never triggered.
+     *
+     * Don't be tempted to restore the guard.  Even if EOF is set on
+     * the channel, continue through and call the inputproc again.  This
+     * is the way to enable the ability to [read] again beyond the EOF,
+     * which seems a strange thing to do, but for which use cases exist
+     * [Tcl Bug 5adc350683] and which may even be essential for channels
+     * representing things like ttys or other devices where the stream
+     * might take the logical form of a series of 'files' separated by
+     * an EOF condition.
      */
 
-    if (GotFlag(statePtr, CHANNEL_EOF)) {
-	return 0;
-    }
-
     /*
      * First check for more buffers in the pushback area of the topmost
      * channel in the stack and use them. They can be the result of a
@@ -6140,6 +6247,7 @@ GetInput(
 
     if (chanPtr->inQueueHead != NULL) {
 
+	/* TODO: Tests to cover this. */
 	assert(statePtr->inQueueHead == NULL);
 
 	statePtr->inQueueHead = chanPtr->inQueueHead;
@@ -6170,6 +6278,7 @@ GetInput(
 	 * Check the actual buffersize against the requested buffersize.
 	 * Saved buffers of the wrong size are squashed. This is done
 	 * to honor dynamic changes of the buffersize made by the user.
+	 * TODO: Tests to cover this.
 	 */
 
 	if ((bufPtr != NULL)
@@ -6713,9 +6822,7 @@ Tcl_Eof(
     ChannelState *statePtr = ((Channel *) chan)->state;
 				/* State of real channel structure. */
 
-    return (GotFlag(statePtr, CHANNEL_STICKY_EOF) ||
-	    (GotFlag(statePtr, CHANNEL_EOF) &&
-	    (Tcl_InputBuffered(chan) == 0))) ? 1 : 0;
+    return GotFlag(statePtr, CHANNEL_EOF) ? 1 : 0;
 }
 
 /*
@@ -8837,6 +8944,36 @@ DoRead(
     ChannelState *statePtr = chanPtr->state;
     char *p = dst;
 
+    assert (bytesToRead >= 0);
+
+    /*
+     * Early out when we know a read will get the eofchar.
+     *
+     * NOTE: This seems to be a bug.  The special handling for
+     * a zero-char read request ought to come first.  As coded
+     * the EOF due to eofchar has distinguishing behavior from
+     * the EOF due to reported EOF on the underlying device, and
+     * that seems undesirable.  However recent history indicates
+     * that new inconsistent behavior in a patchlevel has problems
+     * too.  Keep on keeping on for now.
+     */
+
+    if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) {
+	SetFlag(statePtr, CHANNEL_EOF);
+	assert( statePtr->inputEncodingFlags & TCL_ENCODING_END );
+	assert( !GotFlag(statePtr, CHANNEL_BLOCKED|INPUT_SAW_CR) );
+
+	UpdateInterest(chanPtr);
+	return 0;
+    }
+
+    /* Special handling for zero-char read request. */
+    if (bytesToRead == 0) {
+	ResetFlag(statePtr, CHANNEL_BLOCKED|CHANNEL_EOF);
+	UpdateInterest(chanPtr);
+	return 0;
+    }
+
     TclChannelPreserve((Tcl_Channel)chanPtr);
     while (bytesToRead) {
 	/*
@@ -8848,16 +8985,6 @@ DoRead(
 	ChannelBuffer *bufPtr = statePtr->inQueueHead;
 
 	/*
-	 * When there's no buffered data to read, and we're at EOF,
-	 * escape to the caller.
-	 */
-
-	if (GotFlag(statePtr, CHANNEL_EOF)
-		&& (bufPtr == NULL || IsBufferEmpty(bufPtr))) {
-	    break;
-	}
-
-	/*
 	 * Don't read more data if we have what we need. 
 	 */
 
@@ -8917,8 +9044,7 @@ DoRead(
 	     * 1) We're @EOF because we saw eof char.
 	     */
 
-	    if (statePtr->inEofChar
-		    && RemovePoint(bufPtr)[0] == statePtr->inEofChar) {
+	    if (GotFlag(statePtr, CHANNEL_STICKY_EOF)) {
 		UpdateInterest(chanPtr);
 		break;
 	    }
@@ -8969,17 +9095,33 @@ DoRead(
 		statePtr->inQueueTail = NULL;
 	    }
 	    RecycleBuffer(statePtr, bufPtr, 0);
+	    bufPtr = statePtr->inQueueHead;
 	}
 
 	if (GotFlag(statePtr, CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)
 		== (CHANNEL_NONBLOCKING|CHANNEL_BLOCKED)) {
 	    break;
 	}
+
+	/*
+	 * When there's no buffered data to read, and we're at EOF,
+	 * escape to the caller.
+	 */
+
+	if (GotFlag(statePtr, CHANNEL_EOF)
+		&& (bufPtr == NULL || IsBufferEmpty(bufPtr))) {
+	    break;
+	}
     }
     if (bytesToRead == 0) {
 	ResetFlag(statePtr, CHANNEL_BLOCKED);
     }
 
+	assert(!GotFlag(statePtr, CHANNEL_EOF)
+		|| GotFlag(statePtr, CHANNEL_STICKY_EOF)
+		|| Tcl_InputBuffered((Tcl_Channel)chanPtr) == 0);
+	assert( !(GotFlag(statePtr, CHANNEL_EOF|CHANNEL_BLOCKED)
+		== (CHANNEL_EOF|CHANNEL_BLOCKED)) );
     TclChannelRelease((Tcl_Channel)chanPtr);
     return (int)(p - dst);
 }
diff --git a/generic/tclIOGT.c b/generic/tclIOGT.c
index fe0a880..7ba2f2a 100644
--- a/generic/tclIOGT.c
+++ b/generic/tclIOGT.c
@@ -187,6 +187,7 @@ struct TransformChannelData {
     Tcl_Channel self;		/* Our own Channel handle. */
     int readIsFlushed;		/* Flag to note whether in.flushProc was
 				 * called or not. */
+    int eofPending;		/* Flag: EOF seen down, not raised up */
     int flags;			/* Currently CHANNEL_ASYNC or zero. */
     int watchMask;		/* Current watch/event/interest mask. */
     int mode;			/* Mode of parent channel, OR'ed combination
@@ -292,6 +293,7 @@ TclChannelTransform(
     Tcl_DStringInit(&ds);
     Tcl_GetChannelOption(interp, chan, "-blocking", &ds);
     dataPtr->readIsFlushed = 0;
+    dataPtr->eofPending = 0;
     dataPtr->flags = 0;
     if (ds.string[0] == '0') {
 	dataPtr->flags |= CHANNEL_ASYNC;
@@ -624,7 +626,7 @@ TransformInputProc(
 
     if (toRead == 0 || dataPtr->self == NULL) {
 	/*
-	 * Catch a no-op.
+	 * Catch a no-op. TODO: Is this a panic()?
 	 */
 	return 0;
     }
@@ -676,6 +678,17 @@ TransformInputProc(
 	if (toRead <= 0) {
 	    break;
 	}
+	if (dataPtr->eofPending) {
+	    /*
+	     * Already saw EOF from downChan; don't ask again.
+	     * NOTE: Could move this up to avoid the last maxRead
+	     * execution.  Believe this would still be correct behavior,
+	     * but the test suite tests the whole command callback 
+	     * sequence, so leave it unchanged for now.
+	     */
+
+	    break;
+	}
 
 	/*
 	 * Get bytes from the underlying channel.
@@ -712,14 +725,7 @@ TransformInputProc(
 	     * on the down channel.
 	     */
 
-	    if (dataPtr->readIsFlushed) {
-		/*
-		 * Already flushed, nothing to do anymore.
-		 */
-
-		break;
-	    }
-
+	    dataPtr->eofPending = 1;
 	    dataPtr->readIsFlushed = 1;
 	    ExecuteCallback(dataPtr, NULL, A_FLUSH_READ, NULL, 0,
 		    TRANSMIT_IBUF, P_PRESERVE);
@@ -747,8 +753,11 @@ TransformInputProc(
 	    break;
 	}
     } /* while toRead > 0 */
-    ReleaseData(dataPtr);
 
+    if (gotBytes == 0) {
+	dataPtr->eofPending = 0;
+    }
+    ReleaseData(dataPtr);
     return gotBytes;
 }
 
@@ -859,6 +868,7 @@ TransformSeekProc(
 		P_NO_PRESERVE);
 	ResultClear(&dataPtr->result);
 	dataPtr->readIsFlushed = 0;
+	dataPtr->eofPending = 0;
     }
     ReleaseData(dataPtr);
 
@@ -932,6 +942,7 @@ TransformWideSeekProc(
 		P_NO_PRESERVE);
 	ResultClear(&dataPtr->result);
 	dataPtr->readIsFlushed = 0;
+	dataPtr->eofPending = 0;
     }
     ReleaseData(dataPtr);
 
diff --git a/tests/io.test b/tests/io.test
index f6690ad..d533957 100644
--- a/tests/io.test
+++ b/tests/io.test
@@ -8399,6 +8399,26 @@ test io-73.2 {channel Tcl_Obj SetChannelFromAny, bug 2407783} {} {
     list $code [string map [list $f @@] $msg]
 } {1 {can not find channel named "@@"}}
 
+test io-73.3 {[5adc350683] [gets] after EOF} -setup {
+    set fn [makeFile {} io-73.3]
+    set rfd [open $fn r]
+    set wfd [open $fn a]
+    chan configure $wfd -buffering line
+    read $rfd
+} -body {
+    set result [eof $rfd]
+    puts $wfd "more data"
+    lappend result [eof $rfd]
+    lappend result [gets $rfd]
+    lappend result [eof $rfd]
+    lappend result [gets $rfd]
+    lappend result [eof $rfd]
+} -cleanup {
+    close $wfd
+    close $rfd
+    removeFile io-73.3
+} -result {1 1 {more data} 0 {} 1}
+
 # ### ### ### ######### ######### #########
 
 # cleanup
diff --git a/tests/iogt.test b/tests/iogt.test
index 5fe3dc2..89e62d4 100644
--- a/tests/iogt.test
+++ b/tests/iogt.test
@@ -996,6 +996,78 @@ test iogt-6.1 {Push back and up} {testchannel knownBug} {
     set res
 } {xxxghi}
 
+# Driver for a base channel that emits several short "files"
+# with each terminated by a fleeting EOF
+    proc driver {cmd args} {
+        variable buffer
+        variable index
+        set chan [lindex $args 0]
+        switch -- $cmd {
+            initialize {
+                set index($chan) 0
+                set buffer($chan) .....
+                return {initialize finalize watch read}
+            }
+            finalize {
+                if {![info exists index($chan)]} {return}
+                unset index($chan) buffer($chan)
+                return
+            }
+            watch {}
+            read {
+                set n [lindex $args 1]
+                if {![info exists index($chan)]} {
+                    driver initialize $chan
+                }
+                set new [expr {$index($chan) + $n}]
+                set result [string range $buffer($chan) $index($chan) $new-1]
+                set index($chan) $new
+                if {[string length $result] == 0} {
+                    driver finalize $chan
+                }
+                return $result
+            }
+        }
+    }
+
+test iogt-7.0 {Handle fleeting EOF} -constraints {testchannel} -body {
+    set chan [chan create read [namespace which driver]]
+    identity -attach $chan
+    list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \
+        [read $chan] [eof $chan]
+} -cleanup {
+    close $chan
+} -result {0 ..... 1 {} 0 ..... 1}
+
+proc delay {op data} {
+    variable store
+    switch -- $op {
+	create/write -	create/read  -
+	delete/write -	delete/read  -
+	flush/write -	write -
+	clear_read   {;#ignore}
+	flush/read  -
+	read        {
+	    if {![info exists store]} {set store {}}
+	    set reply $store
+	    set store $data
+	    return $reply
+	}
+	query/maxRead {return -1}
+    }
+}
+
+test iogt-7.1 {Handle fleeting EOF} -constraints {testchannel} -body {
+    set chan [chan create read [namespace which driver]]
+    testchannel transform $chan -command [namespace code delay]
+    list [eof $chan] [read $chan] [eof $chan] [read $chan 0] [eof $chan] \
+        [read $chan] [eof $chan]
+} -cleanup {
+    close $chan
+} -result {0 ..... 1 {} 0 ..... 1}
+
+rename delay {}
+rename driver {}
 
 # cleanup
 foreach file [list dummy dummyout __echo_srv__.tcl] {