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

Jump to bottom

crypto/tls: export/replace errClosed #41066

Closed
ainar-g opened this issue Aug 27, 2020 · 8 comments
Closed

crypto/tls: export/replace errClosed #41066

ainar-g opened this issue Aug 27, 2020 · 8 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals (old)
Milestone
Backlog

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Aug 27, 2020

Following the discussion in #4373, I propose that the currently unexported error errClosed in package crypto/tls be exported. That should simplify error checking for services which use e.g. TCP, TLS, or both types of connection depending on configuration.

@ianlancetaylor also proposed that we replace that custom error with internal/poll.ErrNetClosing, like it is done in that CL, but I am not sure about that, since that could break current code that only wishes to check the error from crypto/tls and not net. On the other hand, checking against one error is obviously easier than checking against two.
@gopherbot gopherbot added this to the Proposal milestone Aug 27, 2020
@ianlancetaylor
Copy link
Contributor

CC @FiloSottile @katiehockman

My suggestion is that we consider simply replacing crypto/tls.errClosed with net.ErrClosed. The error message will be very slightly different, but it will permit consistent use of errors.Is(err, net.ErrClosed) to see whether people are trying to do some operation on a connection has been closed. Or so it seems to me without knowing very much about how this code works.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Aug 27, 2020
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Aug 27, 2020
@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

Now that we've established net.ErrClosed as the "connection is closing/was closed" error, it does seem like TLS should use it.

ping @FiloSottile @katiehockman

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 2, 2020
@rsc rsc mentioned this issue Sep 2, 2020
Open
@katiehockman
Copy link
Contributor

The proposal to use net.ErrClosed in lieu of the unexported errClosed in crypto/tls sounds appropriate. @FiloSottile is also in agreement.

@rsc
Copy link
Contributor

rsc commented Sep 16, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Sep 16, 2020
@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Sep 23, 2020
@ainar-g
Copy link
Contributor Author

ainar-g commented Sep 23, 2020

@rsc Great, thanks! Should I send a CL or wait for someone else to do it? It seems like a small change, but it's still in a crypto package.

@katiehockman
Copy link
Contributor

@ainar-g Feel free to send over a CL with this change if you would like!

@rsc rsc modified the milestones: Proposal, Backlog Sep 23, 2020
@rsc rsc changed the title proposal: crypto/tls: export/replace errClosed crypto/tls: export/replace errClosed Sep 23, 2020
@gopherbot
Copy link

Change https://golang.org/cl/256897 mentions this issue: crypto/tls: replace errClosed with net.ErrClosed

@golang golang locked and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
5 participants
@rsc @ianlancetaylor @ainar-g @gopherbot @katiehockman