bug: Returning pointers to custom errors causes a non-recovered panic

[barak-bem]

Hello! My team recently ran into an issue where returning a nil pointer to a custom error (which we use to attach our own business context) in a Work(...) function caused our whole service to die due to a panic in job_executor.go. We heavily use the custom fields in our error handler.

This can be reproduced like so:

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
}

Totally aware that this is a classic typed nil issue! However I believe it's unfortunate that this can compile and take down the server with very little observability.

We fixed this by adding a check everywhere that looks like this:

if customErr != nil {
     return customErr
}

return nil

Would it be possible to either:

1. Support pointers to custom errors (this is preferred by our team)
2. Recover from the panics within the job executor to avoid bringing down the service

[brandur]

@barak-bem Yeah, man I totally understand that this is annoying as heck. This problem is such a common misunderstanding in Go that they actually added a FAQ for it:

https://go.dev/doc/faq#nil_error

Naturally, it's condescending written and paraphrased basically says "Go is designed perfectly, and some small-minded individuals just don't get it". I would've said: if this so many people are running into trouble with this, it's obviously not a problem with them, but rather a problem with the language's design, and it should be fixed. Alas, it's unlikely to happen at this point.

Regarding River though: given this is much more fundamental problem in Go, I'm not sure that we should have special handling for it. If anything, it should be fixed at a lower level for everyone.

For now, I think the best we could suggest is similar to the code blocks in that FAQ. Instead of writing this:

func returnsError() error {
    var p *MyError = nil
    if bad() {
        p = ErrBad
    }
    return p // Will always return a non-nil error.
}

Write this instead:

func returnsError() error {
    if bad() {
        return ErrBad
    }
    return nil
}

It'd be nice to add a safety hedge or something I suppose, but I'm not even sure it's possible to do so without the use of reflection (i.e. open the pointer to a custom type and check the nil value within).