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.

Sign up for GitHub

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

Jump to bottom

net/url: (*Error).Timeout and (*Error).Temporary methods should use errors.As instead of type assertion #60578

Open
jens1205 opened this issue Jun 2, 2023 · 3 comments
Open

net/url: (*Error).Timeout and (*Error).Temporary methods should use errors.As instead of type assertion #60578

jens1205 opened this issue Jun 2, 2023 · 3 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jens1205
Copy link

jens1205 commented Jun 2, 2023

The url.Error type uses type assertion in the Timeout() and Temporary() method to delegate the work to the wrapped error:

go/src/net/url/url.go

Lines 32 to 44 in 0cd9064

func (e *Error) Timeout() bool {
t, ok := e.Err.(interface {
Timeout() bool
})
return ok && t.Timeout()
}
func (e *Error) Temporary() bool {
t, ok := e.Err.(interface {
Temporary() bool
})
return ok && t.Temporary()
}

In todays time, we shouldn't have implemented the methods like this, but should have left the task of unwrapping the error until some error in the chain implements the wanted interface to the user of the error. But as these methods now are implemented, they should use errors.As to delegate the answer to Timeout() or Temporary() to the next error in the chain who implements this interface - and not only to the direct wrapped error as it is done today.
@mknyszek mknyszek changed the title url.Error Timeout() and Temporary() methods should use errors.As instead of type assertion net/url: (*Error).Timeout and (*Error).Temporary methods should use errors.As instead of type assertion Jun 2, 2023
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jun 2, 2023
@mknyszek mknyszek added this to the Backlog milestone Jun 2, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Jun 2, 2023

CC @neild

@neild
Copy link
Contributor

neild commented Jun 2, 2023

Temporary is deprecated.

Timeout is superseded by specific errors such as os.ErrDeadlineExceeded or context.DeadlineExceeded. (Perhaps it's time to deprecate the Timeout method?)

Should we be making any further changes to the implementations of these methods at this time?

@jens1205
Copy link
Author

Even if Temporary is deprecated and Timeout is about to be deprecated, the current implementation of these methods is wrong, as they are breaking errors.As.

In our codebase, we are deliberately making use of a Temporary method, where we ourselves decide which errors are "temporary". If there is an error being wrapped, and the wrapping error is implementing Unwrap just fine, with this implmentation of url.Error we still don't see with errors.As that somewhere down the error chain there is an error implmenting Temporary or Timeout.

The implementation of these two methods on net.Error is superfluous and incorrect and should not have been done. But as they are now there, even with the deprecation of Temporary and Timeout you still don't want to just delete them (which would be the right solution) as this would be a breaking change. But if you can't delete those methods, they are still active, and IMHO bugs found should be fixed.

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

No branches or pull requests
3 participants
@neild @mknyszek @jens1205