Add nilerror hook to detect nil structs wrapped in non-nil error interfaces#25
Merged
Add nilerror hook to detect nil structs wrapped in non-nil error interfaces#25
nilerror hook to detect nil structs wrapped in non-nil error interfaces#25Conversation
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
that referenced
this pull request
Apr 26, 2025
…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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 26, 2025
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
approved these changes
Apr 28, 2025
Contributor
bgentry
left a comment
There was a problem hiding this comment.
Seems like a solid workaround for people that need it, worth a shot!
nilerror/go.mod
Outdated
Comment on lines
+5
to
+13
| 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 |
Contributor
There was a problem hiding this comment.
Note to not merge this before nuking these replaces and instead pointing at the upcoming River release
brandur
added a commit
to riverqueue/river
that referenced
this pull request
Apr 28, 2025
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
Contributor
Author
|
Pulled the |
brandur
added a commit
that referenced
this pull request
May 3, 2025
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
Merged
brandur
added a commit
that referenced
this pull request
May 3, 2025
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
brandur
added a commit
that referenced
this pull request
May 3, 2025
Prepare release v0.5.0 which largely includes the new `nilerror` package from #25. [skip ci]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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