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

proposal: net: add Unwrap method to *DNSError #63116

Open
mateusz834 opened this issue Sep 20, 2023 · 10 comments · May be fixed by #63348
Open

proposal: net: add Unwrap method to *DNSError #63116

mateusz834 opened this issue Sep 20, 2023 · 10 comments · May be fixed by #63348
Assignees
Labels
Milestone

Comments

@mateusz834
Copy link
Member

mateusz834 commented Sep 20, 2023

Edit: current proposal: #63116 (comment)


This is a proposal to fix the #63109 issue.

I propose adding a causeErr field to DNSError and an Unwrap method that returns the causeErr.

// DNSError represents a DNS lookup error.
type DNSError struct {
    causeErr error // returned by the Unwrap method.
    // (....)     
}

func (e *DNSError) Unwrap() error { return e.causeErr }

This way we keep backwards compatibility for the Error method, we always construct the error from
the Err string field (we keep the current implementation). In the net pacakge we will then make Err field to be the result of the first call to causeErr.Error().

@bcmills
Copy link
Member

bcmills commented Sep 20, 2023

  • Should we do the same for AddrError? (It also has an Err string field.)
  • All fields of DNSError and AddrError are currently exported, so it is possible for packages outside of net to construct one of these errors directly. Should the new field be exported too?

@mateusz834
Copy link
Member Author

mateusz834 commented Sep 20, 2023

Should we do the same for AddrError? (It also has an Err string field.)

AFAIK the AddrError seems to be a standalone error (i.e. it doesn't seem to wrap anything).

All fields of DNSError and AddrError are currently exported, so it is possible for packages outside of net to construct one of these errors directly. Should the new field be exported too?

I don't know whether we want to allow this (we would have to enforce somehow that DNSError.causeErr.Error() == DNSError.Err). Maybe we should leave this for future consideration? We can export it afterwards.

There is still an issue (with the current proposal), that someone might change the Err field without updating the causeErr. (with a *DNSError returned from Lookup functions).

I am unsure about this (in either way).

@ianlancetaylor
Copy link
Contributor

Since all the existing fields of DNSError are exported, I think the new one should also be exported. I suggested Underlying.

@ianlancetaylor
Copy link
Contributor

Here is a test for #63109 that I wrote last night.

func TestCancelBeforeDial(t *testing.T) {
	ln := newLocalListener(t, "tcp")
	defer ln.Close()
	_, port, err := SplitHostPort(ln.Addr().String())
	if err != nil {
		t.Fatal(err)
	}

	ctx, cancel := context.WithCancel(context.Background())
	cancel()
	var d Dialer
	_, err = d.DialContext(ctx, "tcp", JoinHostPort("localhost", port))
	if err == nil {
		t.Error("DialContext with canceled context did not fail")
	} else if !errors.Is(err, context.Canceled) {
		t.Errorf("error.Is(%v, context.Canceled) failed", err)
	}
}

@mateusz834
Copy link
Member Author

Fine, let's make the error field exported.

The proposal looks like this now:

// DNSError represents a DNS lookup error.
type DNSError struct {
	// Underlying is an error that caused this DNSError.
	// It is returned by the Unwrap method.
	//
	// When updating this field make sure to update
	// the Err field accordingly, so that the Error
	// and Unwrap methods don't diverge.
	Underlying error

	// Err is a description of the error, for backwards compatibility
	// the net package sets this error to the value
	// returned by Underlying.Error(), it used by
	// the Error method to produce the error string.
	Err string

	// (...)
}

// Unwrap returns e.Underlying.
func (e *DNSError) Unwrap() error { return e.Underlying }

@mjl-
Copy link

mjl- commented Sep 24, 2023

Having an exported error may also come in handy with possible future support for "extended dns errors", see https://datatracker.ietf.org/doc/html/rfc8914. Could be good to take that into account. It is useful to learn about specifics of dns failures, especially with dnssec errors, and for getting a more descriptive message than "servfail".

@mateusz834
Copy link
Member Author

@mjl- Great idea! I made #63192 for this. This should allow for detailed error messages from the pure go resolver. Not sure how the exported error helps with this. I assume you mean an exported (new) error type just for extended dns errors, I don't think we will ever do this it would be impossible to support this on windows and on unix cgo resolver.

@mjl-
Copy link

mjl- commented Sep 24, 2023 via email

@mateusz834 mateusz834 linked a pull request Oct 3, 2023 that will close this issue
@mateusz834
Copy link
Member Author

mateusz834 commented Oct 3, 2023

I implemented this change in CL 532217, it also simplifies the code a bit with an internal constructor for the *DNSError, so that we don't have to set the Err, IsNotFound, IsTimeout, IsTemporary fields manually each time we construct the error, these fields will be now determined based on the Underlying error type.

Edit: this also implies that the net package will always construct *DNSError with Underlying != nil even for unexported internal errors from the net package, this way we don't overcomplicate this change.

@gopherbot
Copy link

Change https://go.dev/cl/532217 mentions this issue: net: add Unwrap to *DNSError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

Successfully merging a pull request may close this issue.

5 participants