Distinguish cancellation from timeout in DNS. (letsencrypt#5385)

jsha · web-flow · commit 6a8bec395f52 · 2021-04-05T15:44:27.000-07:00
Under normal circumstances, I believe we should never have cause to return a cancellation-related error to the user. This change should distinguish that case in the logs so we can look for it. If it turns out we do sometimes return cancellation-related errors to the user, we should do further digging and figure out why. Related letsencrypt#5346
diff --git a/bdns/dns_test.go b/bdns/dns_test.go
@@ -630,7 +630,7 @@ func TestRetry(t *testing.T) {
 	cancel()
 	_, err := dr.LookupTXT(ctx, "example.com")
 	if err == nil ||
-		err.Error() != "DNS problem: query timed out looking up TXT for example.com" {
+		err.Error() != "DNS problem: query timed out (and was canceled) looking up TXT for example.com" {
 		t.Errorf("expected %s, got %s", context.Canceled, err)
 	}
 
diff --git a/bdns/problem.go b/bdns/problem.go
@@ -32,8 +32,10 @@ func (d Error) Error() string {
 			}
 			// Note: we check d.underlying here even though `Timeout()` does this because the call to `netErr.Timeout()` above only
 			// happens for `*net.OpError` underlying types!
-		} else if d.underlying == context.Canceled || d.underlying == context.DeadlineExceeded {
+		} else if d.underlying == context.DeadlineExceeded {
 			detail = detailDNSTimeout
+		} else if d.underlying == context.Canceled {
+			detail = detailCanceled
 		} else {
 			detail = detailServerFailure
 		}
@@ -60,6 +62,7 @@ func (d Error) Timeout() bool {
 }
 
 const detailDNSTimeout = "query timed out"
+const detailCanceled = "query timed out (and was canceled)"
 const detailDNSNetFailure = "networking error"
 const detailServerFailure = "server failure at resolver"
 
diff --git a/bdns/problem_test.go b/bdns/problem_test.go
@@ -28,7 +28,7 @@ func TestError(t *testing.T) {
 			"DNS problem: query timed out looking up TXT for hostname",
 		}, {
 			&Error{dns.TypeTXT, "hostname", context.Canceled, -1},
-			"DNS problem: query timed out looking up TXT for hostname",
+			"DNS problem: query timed out (and was canceled) looking up TXT for hostname",
 		}, {
 			&Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure},
 			"DNS problem: SERVFAIL looking up CAA for hostname - the domain's nameservers may be malfunctioning",

Original file line number	Diff line number	Diff line change
`@@ -630,7 +630,7 @@ func TestRetry(t *testing.T) {`
`630`	`630`	`cancel()`
`631`	`631`	`_, err := dr.LookupTXT(ctx, "example.com")`
`632`	`632`	`if err == nil \|\|`
`633`		`- err.Error() != "DNS problem: query timed out looking up TXT for example.com" {`
	`633`	`+ err.Error() != "DNS problem: query timed out (and was canceled) looking up TXT for example.com" {`
`634`	`634`	`t.Errorf("expected %s, got %s", context.Canceled, err)`
`635`	`635`	`}`
`636`	`636`
Original file line number	Diff line number	Diff line change
`@@ -32,8 +32,10 @@ func (d Error) Error() string {`
`32`	`32`	`}`
`33`	`33`	// Note: we check d.underlying here even though `Timeout()` does this because the call to `netErr.Timeout()` above only
`34`	`34`	// happens for `*net.OpError` underlying types!
`35`		`- } else if d.underlying == context.Canceled \|\| d.underlying == context.DeadlineExceeded {`
	`35`	`+ } else if d.underlying == context.DeadlineExceeded {`
`36`	`36`	`detail = detailDNSTimeout`
	`37`	`+ } else if d.underlying == context.Canceled {`
	`38`	`+ detail = detailCanceled`
`37`	`39`	`} else {`
`38`	`40`	`detail = detailServerFailure`
`39`	`41`	`}`
`@@ -60,6 +62,7 @@ func (d Error) Timeout() bool {`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`const detailDNSTimeout = "query timed out"`
	`65`	`+const detailCanceled = "query timed out (and was canceled)"`
`63`	`66`	`const detailDNSNetFailure = "networking error"`
`64`	`67`	`const detailServerFailure = "server failure at resolver"`
`65`	`68`