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

awserr: support errors.Is function #4010

Closed
wants to merge 4 commits into from
Closed

Conversation

jkawamoto
Copy link

@jkawamoto jkawamoto commented Jul 15, 2021

It's helpful if we can compare the original error without casting. For example, this usually happens in our use case:

// An API call returns an error.
err := awserr.New("RequestTimeout", "RequestTimeout", context.DeadlineExceeded)

// The error can be wrapped in an upper layer to add more message.
err = fmt.Errorf("additional message: %w", err)

// Then, we need to check what is the original error.
if errors.Is(err, context.DeadlineExceeded){
  ...
}

Unfortunately, the current implementation doesn't support errors.Is. We need to cast the error before the comparision like this:

var e awserr.Error
if errors.As(err, &e) && errors.Is(e.OrigErr(), context.DeadlineExceeded){
  ...
}

However, the above only works if there is one original error. If it has multiple original errors, we need to use BatchedErrors.OrigErrs instead. It makes error handling complicated.

This PR solves this problem. Errors defined in awserr package can work with errors.Is function.

It's helpful if we can compare the original error without casting.
@jasdel
Copy link
Contributor

jasdel commented Aug 17, 2021

Thanks for creating this Pr @jkawamoto there are two older (in progress) PRs covering similar topics, and would be helpful to pull these similar ideas together.

Also, the AWS SDK for Go v2 has solved this problem from the start. It supports errors.Unwrap, and errors.As for all error types.

@jasdel jasdel added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 17, 2021
@jkawamoto
Copy link
Author

@jasdel Thanks for your comment. I added two commits. Commit 0e2d6db adds errors.As support and it should cover #3348 and #2872. Commit 8a95855 also adds Is/As supports to other errors as #3348 (review) mentions.

We'd like to migrate to v2 at some point but some of our depending libraries still use v1. I hope this PR fixes the error issue.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 23, 2021
@jasdel
Copy link
Contributor

jasdel commented Aug 24, 2021

Thanks for updating this pr @jkawamoto . We'll review and post feedback!

@jasdel jasdel self-requested a review August 27, 2021 17:10
Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this Pr @jkawamoto. We'll need to figure out a solution to the SDK's current support of older Go versions prior to Go1.13. Currently this PR's direct usage of errors.As and errors.Is will break with older versions of Go.

One way to handles this is to only add the As, Is functions to the for newer go versions, or alternatively back fill the modern errors As, Is behavior as an internal module to the SDK.

aws/awserr/types.go Outdated Show resolved Hide resolved
@jkawamoto
Copy link
Author

Thanks for your review. I think it's fine to support Is and As functions only for Go 1.13 or later because those functions are mainly called from errors.Is and errors.As, which don't exist before Go 1.13.

@jkawamoto jkawamoto requested a review from jasdel August 30, 2021 19:26
@davidmerwin
Copy link

davidmerwin commented Aug 30, 2021 via email

@jasdel jasdel added the needs-review This issue or pull request needs review from a core team member. label Aug 30, 2021
// sets target to that error value and returns true. Otherwise, it returns false.
func (b baseError) As(target interface{}) bool {
for _, e := range b.errs {
fmt.Printf("%T, %T\n", e, target)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a leftover debugging statement?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've removed it.

@github-actions
Copy link

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness needs-review This issue or pull request needs review from a core team member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants