Add new `HookWorkEnd` interface that runs after workers finish #863

brandur · 2025-04-26T19:54:40Z

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

brandur · 2025-04-26T20:09:16Z

@bgentry ~~I didn't add tests for this one yet. Wanted to get your gut check first in case you disagree with how the error handling works.~~ What do you think?

brandur · 2025-04-26T23:36:13Z

Okay, actually I ended up putting some tests in.

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 · 2025-04-28T05:38:03Z

ty ty.

…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

…terfaces (#25) 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 mentioned this pull request Apr 26, 2025

Add nilerror hook to detect nil structs wrapped in non-nil error interfaces riverqueue/rivercontrib#25

Merged

brandur force-pushed the brandur-hook-work-end branch 2 times, most recently from de63d92 to 61f2272 Compare April 26, 2025 19:59

brandur requested a review from bgentry April 26, 2025 20:08

brandur force-pushed the brandur-hook-work-end branch 3 times, most recently from ce33430 to 1c2173f Compare April 26, 2025 23:34

brandur force-pushed the brandur-hook-work-end branch from 1c2173f to bc8d166 Compare April 26, 2025 23:37

bgentry approved these changes Apr 28, 2025

View reviewed changes

brandur merged commit 9a3f021 into master Apr 28, 2025
10 checks passed

brandur deleted the brandur-hook-work-end branch April 28, 2025 05:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add new `HookWorkEnd` interface that runs after workers finish #863

Add new `HookWorkEnd` interface that runs after workers finish #863

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add new HookWorkEnd interface that runs after workers finish #863

Add new HookWorkEnd interface that runs after workers finish #863

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add new `HookWorkEnd` interface that runs after workers finish #863

Add new `HookWorkEnd` interface that runs after workers finish #863