Skip to content
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.

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

Assign ErrPipeListenerClosed to net.ErrClosed #242

Merged
merged 2 commits into from Apr 20, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Apr 6, 2022

This error used to just be a errors.New with the text exactly matching the "use of closed network connection" that you'd get from the Go stdlib. Now that that error was exported as net.ErrClosed in Go 1.16 we should
be able to swap to this safely.

This error used to just be a `errors.New` with the text exactly matching
the "use of closed network connection" that you'd get from the Go stdlib.
Now that that error was exported as `net.ErrClosed` in Go 1.16 we should
be able to swap to this safely, and anyone who relied on checking the
string explicitly should still be fine.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah requested a review from a team as a code owner April 6, 2022 01:23
@dcantah
Copy link
Contributor Author

dcantah commented Apr 6, 2022

CI's using Go 1.15.5 and that's the reason for the fail, will look in a little. Edit: Our tests didn't specify a go-version, fixed.

Add in a ^1.17.0 go-version for the actions/setup-go@v2 action. This
was set for our build step but not for the step that runs all of the
unit tests.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah
Copy link
Contributor Author

dcantah commented Apr 6, 2022

@thaJeztah I could only see here where moby was making use of this, and it seems like the usage would continue working. Maybe our comment was out of date 🤷‍♂️ https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/libcontainerd/remote/client_io_windows.go#L84-L86.

There's these two spots that check the error text but they're both using tcp/http conns, so not really relevant.

  1. https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/integration/internal/requirement/requirement.go#L20
  2. https://github.com/moby/moby/blob/7b9275c0da707b030e62c96b679a976f31f929d3/cmd/dockerd/metrics.go#L27

@dcantah
Copy link
Contributor Author

dcantah commented Apr 6, 2022

fyi @katiewasnothere @ambarve as I think we'd talked about this before

@dcantah
Copy link
Contributor Author

dcantah commented Apr 20, 2022

@katiewasnothere @ambarve Anybody able to give this a look?

Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

LGTM!

@dcantah dcantah merged commit 5af2f31 into microsoft:master Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants