Skip to content

commandconn: return original error while closing #4394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thaJeztah merged 1 commit into from
Jun 30, 2023
Merged

commandconn: return original error while closing #4394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions cli/connhelper/commandconn/commandconn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Copy link
Member

@thaJeztah thaJeztah Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err may be nil here; do we need to return a io.EOF in that case? (Thinking out loud if we need an io.EOF for calling code)

if err == nil {
    return io.EOF
}

Copy link
Collaborator Author

@laurazard laurazard Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if the read didn't return an error then it's fine to not return one here. This was the original behavior, and subsequent reads will fail regardless with a file closed/pipe closed error.

Copy link
Collaborator Author

@laurazard laurazard Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In normal operation, it will only be closing if an EOF was already returned triggering the transport layer to call Close regardless. This special handling is only meant to keep us from calling handleEOF twice.

Copy link
Member

@thaJeztah thaJeztah Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking! The original comment triggered my curiosity, so wanted to check 😅 👍

}

return n, c.handleEOF(err)
Expand Down
6 changes: 4 additions & 2 deletions cli/connhelper/commandconn/commandconn_unix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package commandconn
import (
"context"
"io"
"io/fs"
"testing"
"time"

Expand Down Expand Up @@ -178,7 +179,8 @@ 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")
Copy link
Member

@thaJeztah thaJeztah Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using one of these for this?

https://github.com/golang/go/blob/18e17e2cb12837ea2c8582ecdb0cc780f49a1aac/src/io/fs/fs.go#L141
https://github.com/golang/go/blob/18e17e2cb12837ea2c8582ecdb0cc780f49a1aac/src/os/error.go#L24

Suggested change
assert.ErrorContains(t, writeErr, "file already closed")
assert.Check(t, is.ErrorIs(writeErr, fs.ErrClosed))

assert.Check(t, is.ErrorIs(writeErr, fs.ErrClosed))
}

func TestCloseWhileReading(t *testing.T) {
Expand Down Expand Up @@ -208,5 +210,5 @@ func TestCloseWhileReading(t *testing.T) {
assert.Check(t, !process.Alive(cmdConn.cmd.Process.Pid))

readErr := <-readErrC
assert.ErrorContains(t, readErr, "EOF")
assert.Check(t, is.ErrorIs(readErr, fs.ErrClosed))
}