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

Do not set err to unwrapped connectErr in error interceptor #3530

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

doriable
Copy link
Member

@doriable doriable commented Dec 11, 2024

This PR removes setting err = connectErr.Unwrap() from our
error interceptor after the connectErr checks. The reason for this
is that this could create some lossiness with err.

For example, when using thread.ParallelizeWithCancelOnFailure,
the errors are appended together with errors.Join:

return errors.Join(errs...)

When we call errors.As(err, connectErr) in the error interceptor, this matches
the first error in the error tree, in this case the context cancellation, and sets that
as the underlying error. So, when connectErr.Unwrap() is called, only the context
cancellation error is returned, and the rest of err is lost.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedDec 12, 2024, 6:19 PM

@@ -471,7 +471,6 @@ func wrapError(err error) error {
}
return errors.New(msg)
}
err = connectErr.Unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

There's a reason we did this - do you know what that is? I don't want to undo it without knowing the reason. I believe it was related to UX ie the error message printed out in this case, but maybe not.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I discussed this with @emcfarlane today, and our thoughts were the reason for this is because connectErr.Unwrap() would remove the connect code from the error message and return the underlying error, e.g. canceled: context canceled -> context canceled. I don't recall any other discussions on this.

The impetus for this change was because there was an issue reported where calling buf generate resulted in a context canceled error because of thread.ParallelizeWithCancelOnFailure. The original err would've returned:

plugin protoc-gen-es: exec: "protoc-gen-es": executable file not found in $PATH
canceled: context canceled

But because we only returned context canceled, it was not easy to debug.

@doriable doriable merged commit f9e5895 into main Dec 12, 2024
10 checks passed
@doriable doriable deleted the do-not-use-connect-err-unwrap branch December 12, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants