-
Notifications
You must be signed in to change notification settings - Fork 22
Update gin-gonic to 1.11.0. #506
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
base: master
Are you sure you want to change the base?
Conversation
Taking inspiration from upstream changes, this modifies the error checking in the recovery middleware to use errors.Is rather than string matching. It also adds http.ErrAbortHandler to the list of errors which should not generate a stack trace.
| err, ok := rec.(error) | ||
| if ok { | ||
| brokenPipe = errors.Is(err, syscall.EPIPE) || | ||
| errors.Is(err, syscall.ECONNRESET) || |
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.
I'm fairly certain this will not be detected on windows, because it's WSAECONNRESET in that case, but that constant is only available when running on Windows, so you can't use it here as it would fail to compile on !windows.
If you wanted to also check that one, there are at least a couple of ways that would work.
Option 1:
One way is to notice that net.OpError.Temporary checks for it because the internal isConnError it calls is specialized per OS, so you could check for that with something like:
var opErr *net.OpError
brokenPipe = errors.Is(err, syscall.EPIPE) ||
errors.Is(err, syscall.ECONNRESET) ||
errors.Is(err, http.ErrAbortHandler) ||
(runtime.GOOS == "windows" && errors.As(err, &opErr) &&
opErr.Temporary())Option 2:
If you'd rather stick with using the syscall.WSAECONNRESET method, you could conditionally compile an isConnResetError that is specialized per OS, similar to how the linked isConnError in the stdlib net package does, and call it from here:
brokenPipe = errors.Is(err, syscall.EPIPE) ||
isConnResetError(err) ||
errors.Is(err, http.ErrAbortHandler)That way, since the code for error_windows.go would only be compiled on Windows, it would be able to do errors.Is(err, syscall.WSAECONNRESET) directly as in:
func isConnResetError(err error) bool {
return errors.Is(err, syscall.WSAECONNRESET)
}While the others would do:
func isConnResetError(err error) bool {
return errors.Is(err, syscall.CONNRESET)
}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.
re option 1, don't use Temporary:
$ go doc net.Error.Temporary
package net // import "net"
type Error interface {
// Deprecated: Temporary errors are not well-defined.
// Most "temporary" errors are timeouts, and the few exceptions are surprising.
// Do not use this method.
Temporary() bool
}
This change all looks super strange to me. I would not expect a web api package to need to be importing syscall at all. Is the existing code broken by the update, or is this just trying to remove string comparisons?
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.
Yep, I was aware of that, but I'm pretty sure that is referring to the interface method in particular. The method on net.OpError has no such restriction documented. Plus, I've looked at its implementation (and linked it in the original comment) on Windows and it's fine to use it for this purpose. That's why I explicitly had the runtime.GOOS == "windows" && errors.As(err, &opErr) part of the check in the possible option.
No description provided.