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
net: document that OpError.Err field must be non-nil #33007
Comments
Thanks for the report. I see that the case of func (e *OpError) Error() string {
if e == nil {
return "<nil>"
}
[...]
} But I'm going to move this issue to the needs decision state. The decision is whether If it should be supported, then it should check if If it shouldn't be supported, perhaps we can make the documentation a little more explicit about that: // OpError is the error type usually returned by functions in the net
// package. It describes the operation, network type, and address of
// an error.
type OpError struct {
[...]
// Err is the error that occurred during the operation.
+ // It must be non-nil.
Err error
} I would argue that it shouldn't be supported, because supporting non-nil /cc @mikioh @ianlancetaylor per owners. |
The |
The whole point of |
Change https://golang.org/cl/187677 mentions this issue: |
Thanks for quick responding! |
@dunjut the issue should remain open until the CL is pushed and Gopherbot closes it. |
The point of *net.OpError is to add details to an underlying lower level error. It makes no sense to have an OpError without an Err and a nil *OpError.Err will cause *OpError.Error() method to panic. Fixes #33007 Change-Id: If4fb2501e02dad110a095b73e18c47312ffa6015 Reviewed-on: https://go-review.googlesource.com/c/go/+/187677 Reviewed-by: Rob Pike <r@golang.org>
Fixed by CL 187677. |
In one of my unit tests, I passed
new(net.OpError)
to the tested function and that function logs the error and does some recovery logic (like re-connect).Logging such err calls
*OpError
'sError()
method, which panics because we didn't check the Err field.Though in most cases we won't get an OpError like this in real scenarios, I suggest maybe checking on Err field still gives better preciseness?
The text was updated successfully, but these errors were encountered: