- Notifications
You must be signed in to change notification settings - Fork329
Use net.ErrClosed#303
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
99ea4f2 tob509f6bCompareAntonboom commentedMay 20, 2021
Like! |
Hi@nhooyr, any chance to get this reviewed? Thanks! |
Ping@nhooyr, would you have the time to have a look? |
Sorry for the delay I'm on a rather long sabbatical right now. Will review when I'm back by the end of this month! |
Cool! No problem, take your time :) |
Antonboom commentedAug 31, 2021
Any news? |
Hi again@nhooyr, any chance to get this reviewed? |
bokunodev left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bothclose__go113.go andclose__go116.go has build tag// +build !go1.16
One is missing a |
Sorry@emersion my sabbatical continues until at least mid October. Apologies for the delay and the broken promise above but I'm dealing with some personal stuff right now and was hoping to be back sooner. I could merge this as is into master as I don't see anything wrong with it but I can't do any sort of release without dealing with#256 first which will take substantial effort. So I'd prefer to leave this PR open as is until I'm back. |
No problem, thanks for having a look.
I'd personally prefer for this to be merged so that I can upgrade to the latest commit even if there's no release yet. But up to you! |
Hi@nhooyr, is it possible to merge this? |
Gentle ping |
@nhooyr, any chance to get this merged? |
Hi, any news about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will get this merged in after#256
write.go Outdated
| func (mw*msgWriter)Write(p []byte) (int,error) { | ||
| ifmw.closed { | ||
| return0,errors.New("cannot use closed writer") | ||
| return0,errClosed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These two are separate errors and should remain. They aren't the same asnet.ErrClosed.
write.go Outdated
| func (mw*msgWriter)Close()error { | ||
| ifmw.closed { | ||
| returnerrors.New("cannot use closed writer") | ||
| returnerrClosed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
.
Hm, why is that? |
Neither of those are closed connections, they're readers and writers for a single frame. There's only the one spot in |
Ah, I see, that makes sense! Fixed. |
rslobodian commentedMar 22, 2023
+1 to this PR :) |
AuroraTea commentedAug 25, 2023
Can this thing go any further? |
Go 1.16 has introduced net.ErrClosed, which should be returned/wrapped when anI/O call is performed on a network connection which has already been closed.This is useful to avoid cluttering logs with messages like "failed to closeWebSocket: already wrote close".Closes:coder#286
Sorry for the delays, thanks@emersion |
Go 1.16 has introduced net.ErrClosed, which should be returned/wrapped when an
I/O call is performed on a network connection which has already been closed.
This is useful to avoid cluttering logs with messages like "failed to close
WebSocket: already wrote close".
Closes:#286