Skip to content

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.

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

Jump to bottom
Merged
merged 1 commit into from
Oct 13, 2023
Merged

Use net.ErrClosed #303

merged 1 commit into from
Oct 13, 2023

Conversation

emersion
Copy link
Contributor

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

@emersion emersion requested a review from nhooyr as a code owner May 19, 2021 21:53
@emersion emersion force-pushed the err-closed branch 2 times, most recently from 99ea4f2 to b509f6b Compare May 19, 2021 21:54
@Antonboom
Copy link

Like!

@emersion
Copy link
Contributor Author

emersion commented Jun 1, 2021

Hi @nhooyr, any chance to get this reviewed? Thanks!

@emersion
Copy link
Contributor Author

Ping @nhooyr, would you have the time to have a look?

@nhooyr
Copy link
Contributor

nhooyr commented Jun 17, 2021

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!

@emersion
Copy link
Contributor Author

Cool! No problem, take your time :)

@Antonboom
Copy link

Any news?

@emersion
Copy link
Contributor Author

Hi again @nhooyr, any chance to get this reviewed?

bokunodev
bokunodev reviewed Sep 13, 2021
View reviewed changes
Copy link

@bokunodev bokunodev left a comment

Choose a reason for hiding this comment

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

both close__go113.go and close__go116.go has build tag
// +build !go1.16

@emersion
Copy link
Contributor Author

One is missing a !.

@nhooyr
Copy link
Contributor

nhooyr commented Sep 19, 2021

Hi again @nhooyr, any chance to get this reviewed?

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.

@emersion
Copy link
Contributor Author

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.

No problem, thanks for having a look.

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.

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!

@emersion
Copy link
Contributor Author

Hi @nhooyr, is it possible to merge this?

@emersion
Copy link
Contributor Author

Gentle ping

@emersion
Copy link
Contributor Author

@nhooyr, any chance to get this merged?

@falmar falmar mentioned this pull request Dec 23, 2022
Merged
@emersion
Copy link
Contributor Author

Hi, any news about this?

nhooyr
nhooyr suggested changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@nhooyr nhooyr left a 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
@@ -53,14 +52,14 @@ type msgWriter struct {

func (mw *msgWriter) Write(p []byte) (int, error) {
if mw.closed {
return 0, errors.New("cannot use closed writer")
return 0, errClosed
Copy link
Contributor

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 as net.ErrClosed.

write.go Outdated
}
return mw.mw.Write(p)
}

func (mw *msgWriter) Close() error {
if mw.closed {
return errors.New("cannot use closed writer")
return errClosed
Copy link
Contributor

Choose a reason for hiding this comment

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

.

@emersion
Copy link
Contributor Author

emersion commented Mar 7, 2023

Hm, why is that? net.Conn returns net.ErrClosed when attempting to write or close an already-closed connection.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 7, 2023

Hm, why is that? net.Conn returns net.ErrClosed when attempting to write or close an already-closed connection.

Neither of those are closed connections, they're readers and writers for a single frame. There's only the one spot in close_notjs.go where it's correctly used when the connection is already closed.

@emersion
Copy link
Contributor Author

emersion commented Mar 7, 2023

Ah, I see, that makes sense! Fixed.

@rslobodian
Copy link

+1 to this PR :)

@AuroraTea
Copy link

Can this thing go any further?
Really needs it.
At least the original error should be exported.
😥

@nhooyr nhooyr changed the base branch from master to dev October 13, 2023 05:19
@emersion @nhooyr
Use net.ErrClosed
e9d0881 
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: coder#286
@nhooyr nhooyr merged commit 7ce08e9 into coder:dev Oct 13, 2023
@nhooyr
Copy link
Contributor

nhooyr commented Oct 13, 2023

Sorry for the delays, thanks @emersion

@nhooyr nhooyr mentioned this pull request Oct 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bokunodev bokunodev bokunodev left review comments

@nhooyr nhooyr nhooyr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use net.ErrClosed
6 participants
@emersion @Antonboom @nhooyr @rslobodian @AuroraTea @bokunodev