Skip to content

Enrich errors caused by context cancellation #91

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

Closed
wants to merge 3 commits into from
Closed

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 28, 2020

Previously the following context.* errors would be returned when the Terraform execution can't start because the context passed was already cancelled.

context deadline exceeded
context canceled

This is being replaced with more informative custom ExitError, e.g.

["/path/to/0.13.1/terraform" "version"] exited: context deadline exceeded

The following exec.ExitError error would be returned when execution is interrupted due to context (cancellation or deadline):

signal: killed

which is being replaced with ExitError too

["sleep" "1"] (pid 71492) exited (code -1): signal: killed
context deadline exceeded

Importantly the ExitError implements Unwrap to remain comparable with the original context errors or exec.ExitError, as demonstrated in attached tests.


We can make deadline errors even more informative by making timeout a first-class concept and implementing custom context.Context which reflects that. Once that is done, the error handling here should in theory just pick it up - only tests may need to be updated. I'll raise a separate PR for this though.

@radeksimko radeksimko force-pushed the ctx-errors branch 2 times, most recently from 86469f4 to b5c21f3 Compare September 29, 2020 11:23
@radeksimko
Copy link
Member Author

Closing in favour of #94

@radeksimko radeksimko closed this Sep 30, 2020
@radeksimko
Copy link
Member Author

#94 does simplify error handling for consumers, but it doesn't actually address any of the above examples of errors.

I think there's still value in this PR, but also I understand if maintaining backwards compatibility for those who don't use errors.Is/As is important, especially if we still maintain Go 1.12 / 1.13 compatibility.

I still think that that default error messaging should be improved and shouldn't be left to each consumer, but maybe it's a topic to revisit later, before we cut v1 and have better justification to make such (potentially) breaking change.

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.

1 participant