Skip to content
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

Closed
dunjut opened this issue Jul 9, 2019 · 7 comments
Closed

net: document that OpError.Err field must be non-nil #33007

dunjut opened this issue Jul 9, 2019 · 7 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dunjut
Copy link

dunjut commented Jul 9, 2019

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's Error() 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?

@dmitshur
Copy link
Contributor

dmitshur commented Jul 15, 2019

Thanks for the report.

I see that the case of *net.OpError being nil is currently supported:

func (e *OpError) Error() string {
	if e == nil {
		return "<nil>"
	}
	[...]
}

But *net.OpError being non-nil with a nil Err field is not (since it panics).

I'm going to move this issue to the needs decision state.

The decision is whether *net.OpError should support users creating it with the Err field set to nil, or if such use is considered invalid and should not be supported. (As far as I can tell, this can only happen if users create *net.OpError themselves, and none of the functions in net package actually return a *net.OpError with nil Err field.)

If it should be supported, then it should check if Err is non-nil before calling Err.Error() (to avoid the panic).

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 *net.OpError with a nil Err field does not make sense and it's not clear what it would represent. If there was network error, then OpError.Err should be non-nil. If there wasn't an error, there shouldn't be a *net.OpError type wrapping it.

/cc @mikioh @ianlancetaylor per owners.

@dmitshur dmitshur added this to the Go1.14 milestone Jul 15, 2019
@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 15, 2019
@dmitshur
Copy link
Contributor

The if e == nil { check was added by @adg in CL 1848049, so I'll /cc him in case he has opinions on this issue.

@ianlancetaylor
Copy link
Contributor

The whole point of OpError is add details to a lower-level error. It is meaningless to have a net.OpError with an Err field that is nil. Let's just document that fact and move on.

@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 15, 2019
@dmitshur dmitshur changed the title net: *OpError.Error() should check its Err field net: document that OpError.Err field must be non-nil Jul 15, 2019
@gopherbot
Copy link

Change https://golang.org/cl/187677 mentions this issue: net: document *OpError.Err must not be nil

@dunjut
Copy link
Author

dunjut commented Jul 29, 2019

Thanks for quick responding!

@dunjut dunjut closed this as completed Jul 29, 2019
@networkimprov
Copy link

@dunjut the issue should remain open until the CL is pushed and Gopherbot closes it.

gopherbot pushed a commit that referenced this issue Jan 19, 2020
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>
@dmitshur
Copy link
Contributor

Fixed by CL 187677.

@golang golang locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants