From 42f94b614a8c353bd0ffed62d558061bcef9e64c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 13 Apr 2023 10:17:24 -0700 Subject: [PATCH] Handle errors once (#179) Adds new guidance on how handling errors once rather than double handling, e.g. log-and-return. Resolves #65 --- CHANGELOG.md | 4 ++ src/SUMMARY.md | 1 + src/error-once.md | 111 +++++++++++++++++++++++++++++++++++++++++++++ style.md | 113 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 229 insertions(+) create mode 100644 src/error-once.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d81ae73d..8498cbc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2023-04-13 + +- Errors: Add guidance on handling errors only once. + # 2023-03-03 - Receivers and Interfaces: Clarify subtlety with pointer receivers and values. diff --git a/src/SUMMARY.md b/src/SUMMARY.md index ff20a6d5..f3b1027e 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -15,6 +15,7 @@ - [Error Types](error-type.md) - [Error Wrapping](error-wrap.md) - [Error Naming](error-name.md) + - [Handle Errors Once](error-once.md) - [Handle Type Assertion Failures](type-assert.md) - [Don't Panic](panic.md) - [Use go.uber.org/atomic](atomic.md) diff --git a/src/error-once.md b/src/error-once.md new file mode 100644 index 00000000..6d91cea9 --- /dev/null +++ b/src/error-once.md @@ -0,0 +1,111 @@ +# Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](error-wrap.md) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + + + + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so causing a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
diff --git a/style.md b/style.md index 644da4e6..7ab980a2 100644 --- a/style.md +++ b/style.md @@ -22,6 +22,7 @@ - [Error Types](#error-types) - [Error Wrapping](#error-wrapping) - [Error Naming](#error-naming) + - [Handle Errors Once](#handle-errors-once) - [Handle Type Assertion Failures](#handle-type-assertion-failures) - [Don't Panic](#dont-panic) - [Use go.uber.org/atomic](#use-gouberorgatomic) @@ -1016,6 +1017,118 @@ func (e *resolveError) Error() string { } ``` +#### Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](#error-wrapping) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + + + + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so causing a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
+ ### Handle Type Assertion Failures The single return value form of a [type assertion](https://golang.org/ref/spec#Type_assertions) will panic on an incorrect