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

🔥 Docs: Error handling #423

Closed
JohannesKaufmann opened this issue May 29, 2020 · 15 comments
Closed

🔥 Docs: Error handling #423

JohannesKaufmann opened this issue May 29, 2020 · 15 comments

Comments

@JohannesKaufmann
Copy link

Echo

I really like echos way of returning an error from your handler, even though I was sceptical at first. What I liked was:

  • that it got rid of the case where an endpoint just returning nothing because I just logged the error and was to lazy to return an error message.
  • forced to handle all error messages, even for c.JSON or c.Render because I had to return something.

It's understandable that you don't want to break the handlers for backwards compatibility.

Fiber: Current State

Your concept of Next and Error does look interesting. But it is a bit difficult piecing together the necessary information (from the Docs, Examples and Github). And just from reading it, I still can't fully understand it 🤷‍♂️

At the same time, there are various code snippets in the Docs or on GitHub were either log.Fatal is used or the error is silently swallowed.

Fiber: Improvements

It would be great to have a page in your docs about how YOU would implement error handling with fiber.

Screenshot 2020-05-29 at 14 51 49


So how could I achieve something similar to echo? Here are the topics that I would like to have on such an "Error handling" page:

  • return a JSON error message {"messsage": "..."} for /api.
  • return an HTML error message for the views.
  • return different status codes based on the error.
  • how to recover from a panic.
  • how to log the error and information about the request (to a different logger).
  • if I call Next, in what order do the handlers execute. How to make sure that my error handler gets called?

These are just the things that come to mind right now. Take it with a grain of salt and maybe ask some other people what is important to them...

@pharrisee
Copy link

pharrisee commented Jun 1, 2020

@JohannesKaufmann I've been playing with the error handling and in lieu of official documentation just thought I'd air some of my findings. The major point of which is that any ErrorHandler you may have must be Use'd last.

So, my app goes in this direction:

  1. Define App
  2. Add global middleware
  3. Define routes
  4. Add error handler
  5. Start server

My ErrorHandler is a fiber middleware, with a simple type that both adds a status code and implements the error interface:

//HTTPError ::::
type HTTPError struct {
	Status int
	Err    error
}

//NewHTTPError ::::
func NewHTTPError(status int, err error) HTTPError {
	return HTTPError{Status: status, Err: err}
}

func (he HTTPError) Error() string {
	return fmt.Sprintf("error %d %s (%s)", he.Status, he.Err.Error(), http.StatusText(he.Status))
}

//ErrorHandler ::::
func ErrorHandler() fiber.Handler {
	return func(c *fiber.Ctx) {
		err := c.Error()
		if err == nil { // no error
			c.Next()
			return
		}
		status := http.StatusInternalServerError //default error status
		if e, ok := err.(HTTPError); ok {        // it's a custom error, so use the status in the error
			status = e.Status
		}
		msg := map[string]interface{}{
			"status":      status,
			"status_text": http.StatusText(status),
			"error_msg":   err.Error(),
		}

		c.Set("Content-Type", "application/json")
		c.Status(status).JSON(msg)
	}
}

This is then used in handlers, like so:

//Root ::::
func (con *Controller) Root(c *fiber.Ctx) {
	if err := c.Render("root", nil); err != nil {
		c.Next(con.NewHTTPError(http.StatusInternalServerError, err))
		return
	}
}

This simple bit of code seems to be doing what I require for the moment, but like you I'd love some guidance as to how the fiber team see error handling working.

I may not be properly understanding something, but this seems like a case whereby in addition to .Use something like a .First and .Last may come in handy. This would mean that we could keep the declarations together whilst maintaining the required order of middleware.

Another option would be a dedicated .ErrorHandler method on the fiber application, which is guaranteed to be called on any error being sent with .Next(err).

@Fenny
Copy link
Member

Fenny commented Jun 1, 2020

@pharrisee I like your idea to add an ErrorHandler setting / method to be called when an error is passed via c.Next()

I don't like the current way how c.Next(err) behaves, it still executes all handlers that match the current route before it hits your custom error handler and I have no short-term solution for that.

Another way would be to create a simple error handler function that is called when needed

func main() {
	app := fiber.New()

	app.Get("/", func(c *fiber.Ctx) {
		if err := c.Render("index", nil); err != nil {
			sadface(c, 500, err)
			return
		}
	})

	app.Listen(3000)
}

func sadface(c *fiber.Ctx, status int, err error) {
	c.Status(status).Send(err.Error())
}

@pharrisee
Copy link

pharrisee commented Jun 1, 2020

If there was an ErrorHandler setting/method on the application, with a default value, then could the .Next method shortcut the rest of the handlers, and also call the ErrorHandler?

https://github.com/gofiber/fiber/blob/master/ctx.go#L588 seems to already check if the caller passed in an error, maybe that could do something to shortcut the rest of the handlers?

@pharrisee
Copy link

Another thought, Gin has a different approach to error handling. It allows multiple errors to be collected in a slice as the handlers proceed. It then has a few methods that shortcut the handlers in different ways, see this issue thread for more info:

gin-gonic/gin#274 (comment)

I'm not advocating fiber do the same, it's just a different way of doing things which may add information.

@Fenny
Copy link
Member

Fenny commented Jun 2, 2020

That looks interesting but maybe we should adopt the express error handling method and use an ErrorHandler that is triggered when calling c.Next(err)

func main() {
	app := fiber.New()

	// Default ErrorHandler
	app.Settings.ErrorHandler = func(ctx *fiber.Ctx) {
		code := StatusInternalServerError
		if e, ok := ctx.Error().(fiber.*Error); ok {
			code = e.Code
		}
		ctx.Status(code).SendString(ctx.Error().Error())
	}

	app.Get("/", func(c *fiber.Ctx) {
		// Triggers ErrorHandler
		c.Next(errors.New("I'm a error!"))
	})

	// This route doesn't get called
	app.Get("/", handler)

	app.Listen(3000)
}

@sujit-baniya
Copy link
Contributor

@Fenny I think the name

app.Settings.HttpErrorHandler = func(err error, c *fiber.Ctx) {
		c.Status(500).Send(err.Error())
	}

makes sense??

@hendratommy
Copy link
Contributor

hendratommy commented Jun 2, 2020

I was thinking this ErrorHandler should have recover functionality by default since it's really crucial for production and I think it's something that default error handler should do rather a middleware. Below is taken from recover middleware, maybe something like:

func defaultErrorHandler() {
    defer func() {
        if r := recover(); r != nil {
            err, ok := r.(error)
            if !ok {
                err = fmt.Errorf("%v", r)
            }
            // Log error  -- Maybe we should have Logger interface like `c.Logger` the same like `fasthttp` ?
            if cfg.Log {
                cfg.Output.Write([]byte(err.Error() + "\n"))
                // c.Logger.Printf("Panic occured: %v", err)
            }

            // Call error handler
            if cfg.ErrorHandler != nil {
                cfg.ErrorHandler(c, &PanicError{err})
            } else {
                c.Status(500).Send(err.Error())
            }
        }
    }()
    c.Next()

    if c.Error() != nil {
        if cfg.ErrorHandler != nil {
            cfg.ErrorHandler(c, c.Error())
        } else {
            c.Status(500).Send(err.Error())
        }
    }
}

Notice that recover will call the ErrorHandler since user might want to have standardized response.
Also we might want to introduce Logger so this logger might be reused across middleware?

@Fenny
Copy link
Member

Fenny commented Jun 3, 2020

@Fenny I think the name

app.Settings.HttpErrorHandler = func(err error, c *fiber.Ctx) {
		c.Status(500).Send(err.Error())
	}

makes sense??

I think we don't need the http prefix since Fiber is an http framework and it would speak for itself 😺

I was thinking this ErrorHandler should have recover functionality by default since it's really crucial for production and I think it's something that default error handler should do rather a middleware. Below is taken from recover middleware, maybe something like:

This is a good point, and I actually had this in mind weeks ago. I will do some testing with @ReneWerner87 to see what the overhead would be if we add this functionality to the router core.

I added ErrorHandler & recover() master...Fenny:master if you want to take a look

@hendratommy
Copy link
Contributor

Cool! Btw, are we going to have some custom errors? Like to differentiate between not found error with any other error?

@Fenny Fenny added this to To do in Development via automation Jun 3, 2020
@Fenny Fenny moved this from To do to In progress in Development Jun 3, 2020
@pharrisee
Copy link

I'm not sure that the recover mechanism should be in the error handler. Since the errorhandler can be overidden by developers that recover functionality would/could be lost?

A separate recover functionality would probably be better?

@Fenny
Copy link
Member

Fenny commented Jun 4, 2020

I will keep you updated on the progress we make, thanks for your valuable feedback 👍

@Fenny
Copy link
Member

Fenny commented Jun 6, 2020

@pharrisee, @hendratommy, @itsursujit, @JohannesKaufmann, thank you guys for participating in this discussion. With the feedback in this issue and on discord we have the following solution regarding error handling for v1.11.x https://docs.gofiber.io/error-handling

Let me know what you think of these changes so far, feedback is always welcome 👍

@Fenny
Copy link
Member

Fenny commented Jun 8, 2020

I'm closing this issue because I just tagged v1.11.x that contains our new ErrorHandler 👍

@Fenny Fenny closed this as completed Jun 8, 2020
Development automation moved this from In progress to Done Jun 8, 2020
@rhzs
Copy link

rhzs commented Jun 5, 2023

@Fenny the doc here https://docs.gofiber.io/error-handling is not working..

@ReneWerner87
Copy link
Member

@Fenny the doc here https://docs.gofiber.io/error-handling is not working..

The new link is https://docs.gofiber.io/guide/error-handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development
  
Done
Development

No branches or pull requests

7 participants