Start adding tests for the interruptable reader

twin/screen-setup-windows.go

@@ -55,10 +55,6 @@ func (r *interruptableReaderImpl) Interrupt() {
 	r.shutdownRequested.Store(true)
 }
 
-func (r *interruptableReaderImpl) Close() error {
-	return nil
-}
-
 func newInterruptableReader(base *os.File) (interruptableReader, error) {
 	return &interruptableReaderImpl{base: base}, nil
 }

twin/screen-setup.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"os"
 	"os/signal"
+	"sync"
 	"syscall"
 
 	log "github.com/sirupsen/logrus"
@@ -20,12 +21,21 @@ type interruptableReaderImpl struct {
 	shutdownPipeReader *os.File
 	shutdownPipeWriter *os.File
 
-	interruptionComplete chan struct{}
+	lock        sync.Mutex
+	interrupted bool
 }
 
 func (r *interruptableReaderImpl) Read(p []byte) (n int, err error) {
 	for {
+		r.lock.Lock()
+		interrupted := r.interrupted
+		r.lock.Unlock()
+		if interrupted {
+			return 0, io.EOF
+		}
+
 		n, err = r.read(p)
+
 		if err == syscall.EINTR {
 			// Not really a problem, we can get this on window resizes for
 			// example, just try again.
@@ -67,9 +77,6 @@ func (r *interruptableReaderImpl) read(p []byte) (n int, err error) {
 
 		err = io.EOF
 
-		// Let Interrupt() know we're done
-		r.Close()
-
 		return
 	}
 
@@ -83,25 +90,21 @@ func (r *interruptableReaderImpl) read(p []byte) (n int, err error) {
 }
 
 func (r *interruptableReaderImpl) Interrupt() {
-	// This will make the select() call claim the read end is ready
-	r.shutdownPipeWriter.Close()
+	r.lock.Lock()
+	defer r.lock.Unlock()
+	r.interrupted = true
 
-	// Wait for reader to react
-	<-r.interruptionComplete
-}
-
-func (r *interruptableReaderImpl) Close() error {
-	select {
-	case r.interruptionComplete <- struct{}{}:
-	default:
+	err := r.shutdownPipeWriter.Close()
+	if err != nil {
+		// This should never happen, but if it does we should log it
+		log.Warn("Failed to close shutdown pipe writer: ", err)
 	}
-	return nil
 }
 
 func newInterruptableReader(base *os.File) (interruptableReader, error) {
 	reader := interruptableReaderImpl{
-		base:                 base,
-		interruptionComplete: make(chan struct{}, 1),
+		base: base,
+		lock: sync.Mutex{},
 	}
 	pr, pw, err := os.Pipe()
 	if err != nil {

twin/screen.go

@@ -74,12 +74,6 @@ type interruptableReader interface {
 
 	// Interrupt unblocks the read call, either now or eventually.
 	Interrupt()
-
-	// Close() should be called after you are done with the interruptableReader.
-	//
-	// It will not close the underlying reader, but it will prevent Interrupt()
-	// from hanging if called after a failure in the screen mainLoop().
-	Close() error
 }
 
 type UnixScreen struct {
@@ -371,10 +365,7 @@ func (screen *UnixScreen) ShowCursorAt(column int, row int) {
 }
 
 func (screen *UnixScreen) mainLoop() {
-	defer func() {
-		screen.ttyInReader.Close()
-		log.Debug("Twin screen main loop done")
-	}()
+	defer log.Debug("Twin screen main loop done")
 
 	// "1400" comes from me trying fling scroll operations on my MacBook
 	// trackpad and looking at the high watermark (logged below).

twin/screen_test.go

@@ -1,8 +1,11 @@
 package twin
 
 import (
+	"io"
+	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"gotest.tools/v3/assert"
 )
@@ -239,3 +242,55 @@ func TestMultiCharHyperlink(t *testing.T) {
 		strings.ReplaceAll(rendered, "", "ESC"),
 		`ESC[mESC]8;;`+url+`ESC\-X-ESC]8;;ESC\ESC[K`)
 }
+
+// Test the most basic form of interruptability. Interrupting and sending a byte
+// should make the reader return EOF.
+//
+// What we really want is for the reader to return EOF immediately when
+// interrupted, with no write needed.
+func TestInterruptableReader_blockedOnRead(t *testing.T) {
+	// Make a pipe to read from and write to
+	pipeReader, pipeWriter, err := os.Pipe()
+	assert.NilError(t, err)
+
+	// Make an interruptable reader
+	testMe, err := newInterruptableReader(pipeReader)
+	assert.NilError(t, err)
+	assert.Assert(t, testMe != nil)
+
+	// Start a thread that reads from the pipe
+	type readResult struct {
+		n   int
+		err error
+	}
+	readResultChan := make(chan readResult)
+	go func() {
+		buffer := make([]byte, 1)
+		n, err := testMe.Read(buffer)
+		readResultChan <- readResult{n, err}
+	}()
+
+	// Give the reader thread some time to start waiting
+	time.Sleep(100 * time.Millisecond)
+
+	// Interrupt the reader
+	testMe.Interrupt()
+
+	// Write a byte to the pipe
+	n, err := pipeWriter.Write([]byte{42})
+	assert.NilError(t, err)
+	assert.Equal(t, n, 1)
+
+	// Wait for the reader thread to finish
+	result := <-readResultChan
+
+	// Check the result
+	assert.Equal(t, result.n, 0)
+	assert.Equal(t, result.err, io.EOF)
+
+	// Another read should return EOF immediately
+	buffer := make([]byte, 1)
+	n, err = testMe.Read(buffer)
+	assert.Equal(t, err, io.EOF)
+	assert.Equal(t, n, 0)
+}