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
lib/nat: Make service termination faster #6777
lib/nat: Make service termination faster #6777
Conversation
return ierr | ||
}) | ||
if err != nil { | ||
if errors.Cause(err) == context.Canceled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does util specifically use github.com/pkg/errors
for wrapping? If so I guess this is fine (but should be flagged for future refactor), otherwise errors.Is
seems more appropriate here now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: pkg/errors is compatible with go1.13's errors.Is/As methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, compatibility was withdrawn in pkg/errors 0.9.1.
package main
import (
"fmt"
"github.com/pkg/errors"
)
func main() {
err := fmt.Errorf("wrapped: %w", fmt.Errorf("inner"))
fmt.Println(errors.Cause(err)) // prints wrapped: inner
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, not so nice - thanks for the hint. The other way around should still work as far as I see (i.e. you can use errors.Is
on an error wrapped with errors.Wrap
). So as we are using fmt.Errorf
in places, we definitely shouldn't use errors.Cause
anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a wider fix that needs to happen through the codebase, hence I am still merging this.
Also, I am not sure this actually affects context.Cancelled, as it's created with errors.New
so does not have Is, so the only way this could go wrong if something wrapped the error which then implemented Is
and explicitly returned false.
Noticed it can take a while