-
Notifications
You must be signed in to change notification settings - Fork 2
Add nilerror hook to detect nil structs wrapped in non-nil error interfaces
#25
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
Conversation
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
…terfaces
This one's aimed at helping people debug problems like the one raised
here [1]. There's a common mistake in Go where a nil error-compliant
struct is wrapped in a non-nil error interface and unexpectedly caught
by a `if err != nil { ... }` check. For example:
type CustomError {
InternalErr error
}
type JobWorker struct {
river.WorkerDefaults[workers.JobWorkerArgs
}
func (jw *JobWorker) Work(ctx context.Context, job *river.Job[workers.JobWorkerArgs]) error {
var customErr *CustomError
return customErr
}
This is such a common problem in Go that the language maintains a long
FAQ on the topic: https://go.dev/doc/faq#nil_error
We probably don't want to bake a solution for this right into core River
because there'd be huge room for disagreement for what to do about it.
Purists would not want the case handled because technically Go is
behaving the way that it's supposed to. Beyond that, in case a problem
is found, it's debatable whether an error should be emitted (so it can
be easily caught in tests) or just logged.
Here, we add a hook in `rivercontrib` as a nice compromise. People who
want extra checks on this problem can easily configure it, and everyone
else can ignore it.
This requires a new `HookWorkEnd` interface in core River to work [1].
It could've been added as a middleware instead, but it seems to me that
a lighter check that doesn't go on the stack in a hook is the better
approach.
[1] riverqueue/river#806
[2] #25
85c3685 to
c7ec4dc
Compare
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
bgentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a solid workaround for people that need it, worth a shot!
nilerror/go.mod
Outdated
| replace github.com/riverqueue/river => ../../river | ||
|
|
||
| replace github.com/riverqueue/river/riverdriver => ../../river/riverdriver | ||
|
|
||
| replace github.com/riverqueue/river/riverdriver/riverpgxv5 => ../../river/riverdriver/riverpgxv5 | ||
|
|
||
| replace github.com/riverqueue/river/rivershared => ../../river/rivershared | ||
|
|
||
| replace github.com/riverqueue/river/rivertype => ../../river/rivertype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to not merge this before nuking these replaces and instead pointing at the upcoming River release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Yep, +1.
Here, add a new complimentary pair for `HookWorkBegin`: `HookWorkEnd`, which runs after workers finish, taking in an error result. `HookWorkEnd` hooks may or may not modify the error result, choosing to suppress an error on pass it along the stack unchanged. This is driven by trying to add a new `nilerror` contrib package [1] that helps detect nil error-compliant structs that return non-nil error interfaces, which is a common footgun in Go [2]. [1] riverqueue/rivercontrib#25 [2] https://go.dev/doc/faq#nil_error
…terfaces
This one's aimed at helping people debug problems like the one raised
here [1]. There's a common mistake in Go where a nil error-compliant
struct is wrapped in a non-nil error interface and unexpectedly caught
by a `if err != nil { ... }` check. For example:
type CustomError {
InternalErr error
}
type JobWorker struct {
river.WorkerDefaults[workers.JobWorkerArgs
}
func (jw *JobWorker) Work(ctx context.Context, job *river.Job[workers.JobWorkerArgs]) error {
var customErr *CustomError
return customErr
}
This is such a common problem in Go that the language maintains a long
FAQ on the topic: https://go.dev/doc/faq#nil_error
We probably don't want to bake a solution for this right into core River
because there'd be huge room for disagreement for what to do about it.
Purists would not want the case handled because technically Go is
behaving the way that it's supposed to. Beyond that, in case a problem
is found, it's debatable whether an error should be emitted (so it can
be easily caught in tests) or just logged.
Here, we add a hook in `rivercontrib` as a nice compromise. People who
want extra checks on this problem can easily configure it, and everyone
else can ignore it.
This requires a new `HookWorkEnd` interface in core River to work [2].
It could've been added as a middleware instead, but it seems to me that
a lighter check that doesn't go on the stack in a hook is the better
approach.
[1] riverqueue/river#806
[2] riverqueue/river#863
|
Pulled the |
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
This one's aimed at helping people debug problems like the one raised
here [1]. There's a common mistake in Go where a nil error-compliant
struct is wrapped in a non-nil error interface and unexpectedly caught
by a
if err != nil { ... }check. For example:This is such a common problem in Go that the language maintains a long
FAQ on the topic: https://go.dev/doc/faq#nil_error
We probably don't want to bake a solution for this right into core River
because there'd be huge room for disagreement for what to do about it.
Purists would not want the case handled because technically Go is
behaving the way that it's supposed to. Beyond that, in case a problem
is found, it's debatable whether an error should be emitted (so it can
be easily caught in tests) or just logged.
Here, we add a hook in
rivercontribas a nice compromise. People whowant extra checks on this problem can easily configure it, and everyone
else can ignore it.
This requires a new
HookWorkEndinterface in core River to work [2].It could've been added as a middleware instead, but it seems to me that
a lighter check that doesn't go on the stack in a hook is the better
approach.
[1] riverqueue/river#806
[2] riverqueue/river#863