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: expose context Canceled/DeadlineExceeded error #36208

Closed
pascaldekloe opened this issue Dec 18, 2019 · 15 comments
Closed

net: expose context Canceled/DeadlineExceeded error #36208

pascaldekloe opened this issue Dec 18, 2019 · 15 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@pascaldekloe
Copy link
Contributor

Issue #28529 could be solved with the new error wrapping now.

What did you do?

Check for context.Canceled with errors.Is seems reasonable.

https://play.golang.org/p/-JTEZXGyfQ6

$ go version
go version go1.13.5 darwin/amd64
@dmitshur dmitshur added this to the Backlog milestone Dec 20, 2019
@dmitshur dmitshur added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 20, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Dec 20, 2019

Thanks for making this issue.

Can you elaborate on what problem it would help solve to expose this information? What is it that cannot be done (or can be done, but inconveniently) before this is resolved, and how will it be done after?

/cc @mikioh @bradfitz @ianlancetaylor

Also /cc @bcmills who commented on the relevant issue #28529.

@pascaldekloe
Copy link
Contributor Author

Generally speaking you want to continue the abort (by returning context.Canceled) if the context expired. Dial errors on the other hand must be reported [logged]. Maybe a retry or two?

@bcmills
Copy link
Contributor

bcmills commented Feb 19, 2020

@pascaldekloe, that doesn't really answer @dmitshur's questions.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 19, 2020
@pascaldekloe
Copy link
Contributor Author

Yes it does @bcmills. You always block my issues with the WaitingForInfo label for some reason. Not helping at all.

@ianlancetaylor
Copy link
Contributor

I believe the point here is to permit using error.Is(err, context.Canceled) on the error returned by net.Dial to see if the Dial failed because the context was canceled. That doesn't work today because of the mapErr function in net/net.go (https://golang.org/src/net/net.go?#L413) which changes context.Canceled to the net package internal error errCanceled. This was done when DialContext was introduced so that the net package operations would continue returning the errors that they always returned. That is because when DialContext was introduced, the existing DialTimeout function was changed to use a context. Changing the error from the internal errCanceled to context.Canceled would certainly break some tests and possible break some real code as well.

So this issue is basically: drop mapErr.

@ianlancetaylor ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 19, 2020
@pascaldekloe
Copy link
Contributor Author

I'm stuck again on the same subject a year later. Would a change request help (to see what the change implies)?

@ianlancetaylor
Copy link
Contributor

Thanks, I think we understand the code change, what we don't understand is what existing code would break if we make this change.

@pascaldekloe
Copy link
Contributor Author

The net.errCanceled mapping is not exported, and its type errors.errorString has no properties other than the textual content. I believe this leaves 4 ways to act upon the error now.

(1) if err.Error() == "dial tcp: operation was canceled"
(2) if err.(*net.OpError).Error() == "operation was canceled"
(3) if strings.Contains(err.Error(), anySubstring("dial tcp: operation was canceled")
(4) if strings.Contains(err.(*net.OpError).Error(), anySubstring("operation was canceled")

If we'd replace net.errCanceled with fmt.Errorf("operation was canceled: %w", ctx.Err()), then only (1) and (2) would break.

Go has issues with exposing errors, i.e. net.ErrClosed, which sometimes forces coders into the practice of string matching. Do we really need compatibility for exact string matching too? That'd be support for bad practice on dubious practice, no?

The respective func mapErr notes // TODO(bradfitz): get rid of this after adjusting tests and making context.DeadlineExceeded implement net.Error?.

@ianlancetaylor
Copy link
Contributor

We know that people do string matching on error strings, and in the past we've decided to not change error strings because doing so broke too much existing code. Yes, matching on error strings is typically bad practice. Still, we want to be careful in this area.

This particular change may be perfectly fine. As I said, we don't what existing code would break if we made the change. Maybe none.

@jayschwa
Copy link
Contributor

Perhaps the error could remain the same but have an Is(target error) bool function added that returns true for the relevant context error.

@pascaldekloe
Copy link
Contributor Author

Do you mean a func net.IsCanceled(error) bool @jayconrod? That could work. 🤔

// silentCauseError wraps an error without affecting the error string (for compatibility reasons). 
type silentCauseError struct {
	s     string
	cause error
}

func (e silentCauseError) Error() string { return e.s }
func (e silentCauseError) Unwrap() error { return e.cause }

In that ☝️ way we could map the errors in a fully compatible way.

func mapErr(err error) error {
	switch err {
	case context.Canceled:
		return silentCauseError{s: errCanceled.Error(), cause: err}
	case context.DeadlineExceeded:
		return timeoutError{cause: err}
	default:
		return err
	}
}

Unless you see any objections I'm happy to make a change request, including the timeoutError extension.

@gopherbot
Copy link

Change https://golang.org/cl/287452 mentions this issue: net: propagate context expiry

@jayschwa
Copy link
Contributor

Do you mean a func net.IsCanceled(error) bool? That could work.

Not quite, I was suggesting to implement interface{ Is(error) bool } on the existing error so that errors.Is(err, context.SomeError) works. It looks like your proposed change will achieve the same effect though. 👍🏻

@pascaldekloe
Copy link
Contributor Author

pascaldekloe commented Jan 27, 2021

That is much better. I missed the Is convention option. The change requests has been updated.

pascaldekloe added a commit to pascaldekloe/mqtt that referenced this issue Feb 9, 2021
ARolek pushed a commit to go-spatial/tegola that referenced this issue Jan 27, 2022
* refactored providers/postgis to use the pgx4 client.
Support for Postgres versions > 12 is now possible.

* provider/postgis: Properly wrap errors in messages by
moving from using %v -> %w when returning errors
in messages.

* Added error string check for context.Canceled.
The underlying net.Dial function is not properly
reporting context.Cancel errors. Becuase of this,
a string check on the error is performed. There's
an open issue for this and it appears it will be
fixed eventually but for now we have this check
to avoid unnecessary logs.

Related issue: golang/go#36208

* added ctxErr() check thewill check if the supplied
context has an error (i.e. context canceled) and if so,
return that error, else return the supplied error.
This is useful as not all of Go's stdlib has adopted
error wrapping so context.Canceled errors are not always
easy to capture.

closes #748
ARolek pushed a commit to go-spatial/tegola that referenced this issue Jan 27, 2022
* refactored providers/postgis to use the pgx4 client.
Support for Postgres versions > 12 is now possible.

* provider/postgis: Properly wrap errors in messages by
moving from using %v -> %w when returning errors
in messages.

* Added error string check for context.Canceled.
The underlying net.Dial function is not properly
reporting context.Cancel errors. Becuase of this,
a string check on the error is performed. There's
an open issue for this and it appears it will be
fixed eventually but for now we have this check
to avoid unnecessary logs.

Related issue: golang/go#36208

* added ctxErr() check thewill check if the supplied
context has an error (i.e. context canceled) and if so,
return that error, else return the supplied error.
This is useful as not all of Go's stdlib has adopted
error wrapping so context.Canceled errors are not always
easy to capture.

closes #748
@pascaldekloe
Copy link
Contributor Author

Got duped by #51428.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants