Skip to content

Conversation

@brandur
Copy link
Contributor

@brandur brandur commented Apr 26, 2025

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

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
@brandur brandur force-pushed the brandur-nilerror branch 2 times, most recently from 85c3685 to c7ec4dc Compare April 26, 2025 19:57
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 brandur requested a review from bgentry April 26, 2025 20:08
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
Copy link
Contributor

@bgentry bgentry left a 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
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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet. Yep, +1.

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
@brandur brandur force-pushed the brandur-nilerror branch from c7ec4dc to 884dd7c Compare May 3, 2025 03:36
…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
@brandur brandur force-pushed the brandur-nilerror branch from 884dd7c to 395e979 Compare May 3, 2025 03:38
@brandur
Copy link
Contributor Author

brandur commented May 3, 2025

Pulled the replace statements and upgraded River.

@brandur brandur merged commit b3f3a15 into master May 3, 2025
2 checks passed
@brandur brandur deleted the brandur-nilerror branch May 3, 2025 03:39
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 brandur mentioned this pull request May 3, 2025
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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants