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

support for deferred error propagation #167

Merged
merged 1 commit into from
Oct 30, 2022

Conversation

mandelsoft
Copy link
Contributor

@mandelsoft mandelsoft commented Oct 27, 2022

What this PR does / why we need it:

A typical problem is to handle errors provided by deferred functions, for example Close().

Another scenario is the need to wrap all errors provided in a function with a common context description.

The errors package now provides a function PropagateError/f, which can be used to solve both problems.

If takes an optional error context, which will be used to wrap a potential error returned by the calling function. Additionally it is possible to add a function providing an error. This error, if present is composed with an error provided by the function return when called as deferred function.

Example:

func() (efferr error) {
  ...
  defer errors.PropagateErrorf(&efferr, stream.Close, "error context")
  ...
  return err
}

This error providing function may be the Finalize() method of a utils.Finalizer.

This is now directly supported by the Finalizer.

Example:

func() (efferr error) {
  var finalize utils.Finalizer
  ...
  defer finalize.FinalizeWithErrorPropagationf(&efferr, "error context")
  ...
  finalize.Close(stream)
  ...
  return err
}

For more variants see the tests in the errors and utils package.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

A typical problem is to handle errors provided by deferred functions,
for example Close().

Another scenario is the need to wrap all errors provided in a function
with a common context description.

The errors package now provides a function PropagateError, which
can be used to solve both problems.

If takes an optional error context, which will be used to wrap
a potential error returned by the calling function.
Additionally it is possible to add a function providing
an error. This error, if present is composed with an error
provided by the function return when called as deferred function.

Example:

func() (efferr error) {
  ...
  defer errors.PropagateErrorf(&efferr, stream.Close, "error context")
  ...
  return err
}

This error providing function may be the Finalize() method of a
utils.Finalizer.

This is now directly supported by the Finalizer.

Example:

func() (efferr error) {
  var finalize utils.Finalizer
  ...
  defer finalize.FinalizeWithErrorPropagationf(&efferr, "error context")
  ...
  finalize.Close(stream)
  ...
  return err
}
@Skarlso
Copy link
Contributor

Skarlso commented Oct 27, 2022

Why not use re-assignment of a deferred error? Or just return multiple errors? Something like:

func test() (err error) {
	defer func() {
		if tmpErr := doSomething(); tmpErr != nil {
			err = tmpErr
		}
	}()
	
	err = doSomethingElse()
	
	return err
}

Or append to an error list.

return false
}

func Is(err error, target error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why Go's errors.Is and errors.As is reimplemented here but slightly differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original Isand As functions do not care about error lists. This is what the new implementations add.

@mandelsoft
Copy link
Contributor Author

@Skarlso, overwriting an error would loose the original error (of interest). But what I'm doing is exactly your second proposal,
returning an error list.

@Skarlso
Copy link
Contributor

Skarlso commented Oct 27, 2022

@Skarlso, overwriting an error would loose the original error (of interest).

Sure, it depends on if you are more interested in the finalizer failing or not. If not at all, log the failure and move on.
If you still care about the original, just chain them with fmt.Errorf("finalizer failed with %w while handling call to X that failed with %w, tmpErr, err). Then you can do an errors.Is on it and get the revelant error.

But what I'm doing is exactly your second proposal, returning an error list.

Well, technically, you are hiding the error list inside a parameter which is handled as output. That is usually not considered a good way of handling return values. This is typically done by unmarshallers or kube client which sets an output value. Typically, a more straightforward way would be to expose this behaviour. But I know that it would result in a massive rewrite returning errors everywhere. :)

@mandelsoft
Copy link
Contributor Author

Hmm. I' not sure, what you mean with overwriting an output (here errror) is usually not considered a good way. You have to do it, if you want to handle and return a deferred error. I found examples exactly explaining this mechanism. If you don't want to bundle the errors (occurs only, if the method as well as the finializers will fail), you don't need to use this function and do it according to your special needs. It is just some convenience function, which provides a standard error handling, which is now easy to consume, without the need of complex boiler plate.

The problem with the vanished error during the signing process is just such a case, the error occurs during a The Close of the component version and should be reported.

@Skarlso
Copy link
Contributor

Skarlso commented Oct 28, 2022

You have to do it, if you want to handle and return a deferred error

You don't have to. But you can. :) Anyways, this is fine; I just wanted to know what was behind it.

@mandelsoft mandelsoft marked this pull request as ready for review October 30, 2022 16:11
@mandelsoft mandelsoft merged commit 852bdfc into open-component-model:main Oct 30, 2022
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.

3 participants