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

proposal: errors: configurable Handle(error) function #33162

Open
mvndaai opened this issue Jul 17, 2019 · 24 comments
Open

proposal: errors: configurable Handle(error) function #33162

mvndaai opened this issue Jul 17, 2019 · 24 comments

Comments

@mvndaai
Copy link

@mvndaai mvndaai commented Jul 17, 2019

Problem

Most of the time in Go if an error occurs it can be returned. There are certain cases when this is not possible, usually go functions.

go func(){
    if err := foo(); err != nil {
        // cannot 'return err' because the function has already returned
    }
}

Packages shouldn't choose how to handle errors so they end up either squashing the error or making configurable error logging like ErrorLog in httputil.ReverseProxy or http.Server.

Proposal

I propose adding a function errors.Handle(error) that can be called in cases where it is impossible to return an error.

There should be a second function errors.SetHandler(func(error)) that can be called, usually in main, that lets an application choose how to handle an error. (i.e. log, increment a metrics, ...)

@gopherbot gopherbot added this to the Proposal milestone Jul 17, 2019
@gopherbot gopherbot added the Proposal label Jul 17, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 17, 2019

One of the most common ways to handle an error is to return it. Am I correct in thinking that that is not supported here?

It's very common for different functions to want to handle errors differently, but it seems that errors.SetHandler is global, and would only be called once per program. While that might be workable within a single application, I'm skeptical that library code would be able to use errors.Handler.

@ianlancetaylor ianlancetaylor changed the title Proposal: Configurable errors.Handle(error) function proposal: errors: configurable Handle(error) function Jul 17, 2019
@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 18, 2019

@ianlancetaylor thanks for taking the time to review this!

As a clarification, returning an error does not handle it, it allows the parent function to handle it instead. This would not change that flow at all, it would only change the moments when errors are actually handled.

You are correct that errors.SetHandler would be global. It is not meant to be used by the library function but instead let applications configure it if they want.

The point of this change is that the library code would use error.Handle in cases when an error is handled instead of returned. Currently, code like defer f.Close() just ignores that an error could exist. Having error.Handle would mean that the library packages do not need to be opinionated on how errors are handled.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 18, 2019

To clarify

library packages do not need to be opinionated on how errors are handled.

There are many ways to handle an error and all are valid depending on the application. Here are some examples:

log.Println(err)
fmt.Println(err)

https://cloud.google.com/error-reporting/docs/setup/go

errorClient.Report(errorreporting.Entry{
    Error: err,
})
log.Print(err)

https://cloud.google.com/logging/docs/setup/go

logger := client.Logger(logName).StandardLogger(logging.Error)
logger.Println(err)

This change means that library packages and all packages just call errors.Handle and don't have to care about preference, but instead leave it up to the application.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 18, 2019

It seems to me that a library can let an application handle an error by simply returning the error.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 18, 2019

@ianlancetaylor thank you for taking time on this. This proposal is for instances when it impossible to

simply return the error

Since that was unclear I went back and edited the proposal to hopefully be more clear. Thanks again!

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Jul 18, 2019

It used to be very common to write programs that work by setting global values, rather than returning things. You could still do this today with:

package globalerr

var Error error

func IsSet() bool {
    return Error != nil
}

And then just set this when you want to in your functions and methods. I don't think this is a good idea, however. This style of programming turns out to compose poorly and be quite brittle. I don't think there's any reason for the language to encourage this approach to errors.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 18, 2019

@carlmjohnson this isn't about having global errors. This is giving a way for packages to handle ones that are currently getting ignored.

In the function ioutil.ReadFile there is an ignored error in defer f.Close()

func ReadFile(filename string) ([]byte, error) {
	f, err := os.Open(filename)
	if err != nil {
		return nil, err
	}
	defer f.Close()

Although closing does not affect the flow of the program the error shouldn't be swallowed. I should know that errors are happening. This could be a memory leak in my program.

Currently, if ioutil wanted to handle the error and not change the flow of the program it would have to be opinionated.

defer func(){
    if err := f.Close(); err != nil {
        log.Println("file was not closed", err) // Using `log` is being opinionated
    }
}()

Making a generic errors.Handle(err) function means ioutil does not need to be opinionated

defer func(){
    if err := f.Close(); err != nil {
        errors.Handle(err)
    }
}()

The default for errors.Handle could be log.Println(err.Error()), but if my application uses a different way of handling errors errors.SetHandler could be used.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 18, 2019

In my applications, my error handler function is very complicated and opinionated. It has multiple steps:

  1. Determine the severity of an error if I can
  2. Log the error in Stackdriver using the determined severity
  3. Increment a metric

I would not want ioutils or any other package that I use to determine how to handle errors. If they used log.Println it would be completely lost in my logs and not displayed in my metrics. If there was an errors.Handle function that those packages could use it would not get lost.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 19, 2019

A much better example is ErrorLog in httputil.ReverseProxy{}

If nil, logging goes to os.Stderr via the log package's standard logger.

The httputil packaged created this function

func (p *ReverseProxy) logf(format string, args ...interface{}) {
	if p.ErrorLog != nil {
		p.ErrorLog.Printf(format, args...)
	} else {
		log.Printf(format, args...)
	}
}

I believe that log shouldn't be a dependency of httputil and calls to that function should be replaced by errors.Handle.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Jul 31, 2019

Based on the comments the proposal was unclear. I rewrote it to be more to the point. Thank you everyones for taking the time to review.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2019

I think we should decline all the error handling proposals until we are ready to revisit the topic (perhaps not for a few years).

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Dec 1, 2019

@rsc can you categorize this differently than all the other error handling ones. This is NOT a different approach to try, decoration, and return flow. This what to do with an error when it gets to the top of the return tree.

@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc rsc added the Go2 label Feb 5, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Feb 5, 2020

Moving to Go2 to be with all the other error handling proposals.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Feb 6, 2020

@rsc this isn't a change like try or the boilerplate of if err != nil {. There are no changes needed to the language. The paradigm of decorate and return doesn't change. If you have to label it Go2 to not close it, I guess that is better than closing this, but it isn't really necessary.

This is a fix for a missing feature of Go on how to make a decision on what to do with an error if it cannot be returned (go and defer func returns). It only requires adding 2 functions to the current errors package.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 6, 2020

I appreciate that you feel strongly about this issue.

I can't think of anything else in the Go standard library that works this way. The idea of a global handler just doesn't seem very Go like to me. This feature is only useful if the standard library itself uses it, but I think that for any specific case in the standard library we would look for some other way to return the error to the application.

What I learn from your example of ioutil.ReadFile is that we should fix that function, not that we should add a global error handling mechanism.

You suggest that shouldn't want to be opinionated, but in fact Go is an opinionated language, by choice. And the opinionated choice is to return errors. httputil.ReverseProxy is indeed an exception here.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Feb 7, 2020

@ianlancetaylor Thanks for the kind words and taking the time to respond. That being said I disagree that

This feature is only useful if the standard library itself uses it

It would be amazing if the standard library used it, but it would be useful for any imported package.

On the comment of

You suggest that shouldn't want to be opinionated, but in fact Go is an opinionated language, by choice. And the opinionated choice is to return errors

I agree that the Go choice is to return errors and I don't want to change that choice. Since Go has defer and go functions, that isn't always possible. I want Go to be opinionated on how to handle errors, I just don't want every other package I try to import from github to have their own opinions.

I went looking through a few lists of popular go library mostly searching for defer and go funcs to see how errors were handled. This code from the asw-sdk-go code is the best example of why this is needed.

// TODO: What to do with this error? Probably should just log
b, err := json.Marshal(m)
if err != nil {
	continue
}

Here are some other examples of how packages have chosen to handle errors were errors.Handle might be better.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 7, 2020

@mvndaai, I think the issue you're pointing to is real, but I'm not sure a global error handler is a good solution. Different errors require different responses—log, ignore, panic (or abort, retry, fail if you remember the days of DOS)—and one handler can't be expected to tell which case is which.

For deferred writes specifically, I and many other have a simple helper function. I think something like it should be considered for the standard library errors package. For problems with goroutines not reporting errors, I think a more systematic solution would be needed—possibly in an entirely new programming language.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Feb 7, 2020

@carlmjohnson thanks for understanding that this is a real issue.

Thanks for pointing out a way to handle deferred errors. Although that is helpful if people want to change how they are writing code and use named returns, that is still ignoring all go functions and other errors like those from channels.

I agree that different errors deserver different responses. I am not advocating that this be used instead of return error. That would be a horrible idea. I want errors.Handle to be configurable so I could use errors.Is to decide what to do differently if I need something different, only in situations where a returned error is not possible.

The issue is that are currently lots of places where errors cannot be returned. Imported packages, which I cannot control, have channels, go funcs, and defer funcs. Applications, which we can actually control, have others like main or http handler funcs.

The things in my control I can handle how I want. The ones that are not, have no standard way of being handled in go. They are usually a panic from inexperienced developers because that is what all the tutorials teach. All other examples I have seen either log using fmt/log or just ignore them.

@fwhezfwhez
Copy link

@fwhezfwhez fwhezfwhez commented Mar 6, 2020

I'm using this code style to handle error:
./main.go

package main

import (
	"fmt"
	"src"
)

func main() {
	if e := src.AddUser(); e != nil {
		fmt.Println(e.Error())
		return
	}
}

./src/service.go

package src

import (
	"fmt"
	"github.com/fwhezfwhez/errorx"
)

func AddUser() error {
	if e := generateError(); e != nil {
		return errorx.Wrap(e)
	}
	return nil
}
func generateError() error {
	var e = fmt.Errorf("time out")
	return errorx.Wrap(e)
}

output:

2020-03-06 14:31:00 | G:/go_workspace/GOPATH/src/mapx/src/service.go: 16 | time out
2020-03-06 14:31:00 | G:/go_workspace/GOPATH/src/mapx/src/service.go: 10 | time out
@mvndaai
Copy link
Author

@mvndaai mvndaai commented Mar 6, 2020

@fwhezfwhez that package seems helpful, thanks for bringing it to my attention, but it does not solve a main issue of this proposal: What happens when a package you import has an error in a defer or a go func? In both of those cases errorx might not be used, so you would have logs that don't fit the rest of your logs if they package chooses to log it at all.

@fwhezfwhez
Copy link

@fwhezfwhez fwhezfwhez commented Mar 9, 2020

@mvndaai errorx aims to return errors trace-wrapped and handle them in an unified place.It doesn't conflict whether handle errors in defer part or go func() part.

func service() error {
    defer func() {
        if e:= destroyObject(); e!=nil {
            handle(errorx.Wrap(e))
        }
        if e:= x.Close(); e!=nil {
            handle(errorx.Wrap(e))
        }
    }()
    e:= generateError("time out")
    return errorx.Wrap(e)
}
 go func(){
    if e:= service(); e!=nil {
        handle(errorx.Wrap(e))
    }
}()

Apparently in defer part, errors can't return, thus you might handle it straightly.
So does go part.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Mar 10, 2020

@fwhezfwhez I am confused. I agree that you can call whatever handle function in your own code. The point of this change is that there should be an errors.Handle to call instead, because in a package that you import and they used a go func, they don't have access to your handle function.

@fwhezfwhez
Copy link

@fwhezfwhez fwhezfwhez commented Mar 10, 2020

@fwhezfwhez I am confused. I agree that you can call whatever handle function in your own code. The point of this change is that there should be an errors.Handle to call instead, because in a package that you import and they used a go func, they don't have access to your handle function.

It requires package itself provides xxx.SetErrorHandle(f func(error)) then you can handle error inner a package.
If a package handle error inside and do not return, it means this error is not exported as expected.I think no need to care about an error the author do not want you to handle.

@mvndaai
Copy link
Author

@mvndaai mvndaai commented Mar 10, 2020

@fwhezfwhez I would rather have an errors.SetErrorHandle(func(error)) instead of needing to call a function for every package I import. Then all authors can just call errors.Handle. I want to choose if I care about an error, not defer to the author of the package.

@rsc rsc removed this from Incoming in Proposals Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.