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
Comments
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. |
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? |
@pascaldekloe, that doesn't really answer @dmitshur's questions. |
Yes it does @bcmills. You always block my issues with the WaitingForInfo label for some reason. Not helping at all. |
I believe the point here is to permit using So this issue is basically: drop |
I'm stuck again on the same subject a year later. Would a change request help (to see what the change implies)? |
Thanks, I think we understand the code change, what we don't understand is what existing code would break if we make this change. |
The (1) If we'd replace 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 |
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. |
Perhaps the error could remain the same but have an |
Do you mean a // 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 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. |
Change https://golang.org/cl/287452 mentions this issue: |
Not quite, I was suggesting to implement |
That is much better. I missed the Is convention option. The change requests has been updated. |
* 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
* 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
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
The text was updated successfully, but these errors were encountered: