-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add argument to WebsocketErrorFunc #2124
Add argument to WebsocketErrorFunc #2124
Conversation
to determine whether the error ocured on read or write to the websocket.
@telemenar @RobinCPel What are your thoughts on this? On the one hand, changing the function signature breaks backwards compatibility, and I would rather not do that unless it was extremely important and we couldn't find another way. |
Hmmm, how I handle the error is by type switching it like so: switch e := err.(type) {
case *websocket.CloseError:
if e.Code == websocket.CloseNormalClosure || e.Code == websocket.CloseGoingAway {
// Either a "normal close", or a "going to close" message, not actually an error
return
}
case net.Error:
if e.Temporary() {
// The error is temporary... so it should fix itself, right?
return
}
default:
if err == websocket.ErrCloseSent {
// We closed the connection, but we still tried to read/write, so everything is probably fine!
return
}
// The websocket.ErrReadLimit and websocket.ErrBadHandshake errors might also end up here
// We can't identify the error => assume it is an error that is fatal for the web socket connection
} But I believe that this (unfortunately) does not supply enough info to determine whether the error occurred on read or write... so this PR would probably be a good addition! Breaking the code is not optimal... but the error feature is still kinda new, and the change is not that large... so I would say let's just do it! Something that also came to mind: Should we maybe - instead of adding this boolean - add something that we can expand when the need arises? So we don't have to break it two times? Maybe a custom error that is wrapped around the actual web socket error that can also contain this boolean? Then it would definitely break this function even harder, but it would also future-proof it 🤔 |
@StevenACoffman @RobinCPel FYI I've implemented the error wrapper suggestion in the second commit. |
If you can also add a test for a write error, you'll have my approval! (a second opinion would be great) |
Thanks! Looks good to me, but I would also like to have a test for a write error if possible! |
@RobinCPel @StevenACoffman that sounds good but do you have an idea of how a write error can be emulated? |
Nope 😅 |
I do, but I am not able to spend the time on doing it, so I'm going to just merge it as is, going by "if you aren't willing to do something yourself, it's unreasonable to ask that of others" |
Thanks for this contribution! |
It might be useful in
WebsocketErrorFunc
to understand whether the error occurred on reading or writing to the WebSocket.This PR adds an argument for this to
WebsocketErrorFunc
. As an alternative to this, I'd suggest splitting the error func into two:WebsocketWriteErrorFunc
andWebsocketReadErrorFunc
.I have: