commandconn: return original error while closing

laurazard · laurazard · commit ee888c699d99 · 2023-06-30T15:03:19.000+01:00
Changes the `Read` and `Write` error handling
logic to return the original error while closing
the connection. We still skip calling `handleEOF`
if already closing the connection.

Fixes the flaky `TestCloseWhileWriting` and
`TestCloseWhileReading` tests.

Signed-off-by: Laura Brehm &lt;laurabrehm@hey.com&gt;
diff --git a/cli/connhelper/commandconn/commandconn.go b/cli/connhelper/commandconn/commandconn.go
@@ -163,9 +163,8 @@ func (c *commandConn) Read(p []byte) (int, error) {
 	// Close might get called
 	if c.closing.Load() {
 		// If we're currently closing the connection
-		// we don't want to call onEOF, but we do want
-		// to return an io.EOF
-		return 0, io.EOF
+		// we don't want to call onEOF
+		return n, err
 	}
 
 	return n, c.handleEOF(err)
@@ -178,9 +177,8 @@ func (c *commandConn) Write(p []byte) (int, error) {
 	// Close might get called
 	if c.closing.Load() {
 		// If we're currently closing the connection
-		// we don't want to call onEOF, but we do want
-		// to return an io.EOF
-		return 0, io.EOF
+		// we don't want to call onEOF
+		return n, err
 	}
 
 	return n, c.handleEOF(err)
diff --git a/cli/connhelper/commandconn/commandconn_unix_test.go b/cli/connhelper/commandconn/commandconn_unix_test.go
@@ -178,7 +178,7 @@ func TestCloseWhileWriting(t *testing.T) {
 	assert.Check(t, !process.Alive(cmdConn.cmd.Process.Pid))
 
 	writeErr := <-writeErrC
-	assert.ErrorContains(t, writeErr, "EOF")
+	assert.ErrorContains(t, writeErr, "file already closed")
 }
 
 func TestCloseWhileReading(t *testing.T) {
@@ -208,5 +208,5 @@ func TestCloseWhileReading(t *testing.T) {
 	assert.Check(t, !process.Alive(cmdConn.cmd.Process.Pid))
 
 	readErr := <-readErrC
-	assert.ErrorContains(t, readErr, "EOF")
+	assert.ErrorContains(t, readErr, "file already closed")
 }

Original file line number	Diff line number	Diff line change
`@@ -178,7 +178,7 @@ func TestCloseWhileWriting(t *testing.T) {`
`178`	`178`	`assert.Check(t, !process.Alive(cmdConn.cmd.Process.Pid))`
`179`	`179`
`180`	`180`	`writeErr := <-writeErrC`
`181`		`- assert.ErrorContains(t, writeErr, "EOF")`
	`181`	`+ assert.ErrorContains(t, writeErr, "file already closed")`
`182`	`182`	`}`
`183`	`183`
`184`	`184`	`func TestCloseWhileReading(t *testing.T) {`
`@@ -208,5 +208,5 @@ func TestCloseWhileReading(t *testing.T) {`
`208`	`208`	`assert.Check(t, !process.Alive(cmdConn.cmd.Process.Pid))`
`209`	`209`
`210`	`210`	`readErr := <-readErrC`
`211`		`- assert.ErrorContains(t, readErr, "EOF")`
	`211`	`+ assert.ErrorContains(t, readErr, "file already closed")`
`212`	`212`	`}`