commandconn: return original error while closing

[laurazard]

• fixes Flaky test: TestCloseWhileWriting, TestCloseWhileReading #4356

- What I did

Fixes the flaky TestCloseWhileWriting and TestCloseWhileReading tests.

- How I did it

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.

- How to verify it

go test -v ./..., and make sure everything still works.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

Merging #4394 (ee888c6) into master (cb1def7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4394      +/-   ##
==========================================
- Coverage   59.37%   59.37%   -0.01%     
==========================================
  Files         288      286       -2     
  Lines       24746    24746              
==========================================
- Hits        14693    14692       -1     
  Misses       9169     9169              
- Partials      884      885       +1     

[thaJeztah]

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
}

[laurazard]

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.

[laurazard]

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.

[thaJeztah]

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

[thaJeztah]

minor comment on the test, but otherwise LGTM

[thaJeztah]

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))

[thaJeztah]

Suggested change

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

[thaJeztah]

I applied my suggestions in the test

LGTM

[laurazard]

Thanks! I was away from a keyboard, appreciate it ❤️

[thaJeztah]

Preparing a cherry-pick; will be up in a minute 👍

[thaJeztah]

• Backport is up; [24.0 backport] ssh: fix error on commandconn close, add ping and default timeout  #4395

[vvoland]

Looks like it's still flaky - I just got a test failure in a freshly rebased PR: https://github.com/docker/cli/actions/runs/5454066267/jobs/9923693780?pr=4331

 85.59 === Failed
85.59 === FAIL: cli/connhelper/commandconn TestCloseWhileWriting (0.00s)
85.59     commandconn_unix_test.go:182: assertion failed: expected error to contain "file already closed", got "write |1: broken pipe"

\cc @laurazard

[laurazard]

Thanks @vvoland. I'll TAL when I get a change.