When a panic occurs, the middleware execution is not completed

[jerbob92]

When triggering a panic from a worker, this panic will only cause HandlePanic in the error handler to be called, but it will not continue execution in the middleware. I would like to see this behaviour changed so that panics are just a typed error after HandlePanic.

Without this, the middleware will never see the panic so then it will also not reported to Sentry as error and/or transaction without duplicating the code.

I also noticed that the context that is given in the panic handler is different and doesn't contain the context added by the middleware.

[brandur]

Hey @jerbob92,

Go-wise, it'd be a little on the odd side to just directly convert a panic to an error and return that. Panics and errors are both error conditions, but two distinct ways that an error may occur.

The middleware should be in the stack that's unraveling as the panic occurs, but before the job executor handles it. Did you consider having an alternate recover block inside your middleware? i.e. Something like:

func (m *ErrorRecoveryMiddleware) Work(ctx context.Context, job *rivertype.JobRow, next func(context.Context) error) error {
	defer func() {
        // handle panic (i.e. send to Sentry) here

		if recovery := recover(); recovery != nil {
            // panic again to make sure the right value goes into the job row
            panic(recovery)
        }
    }()


	err := next(ctx)

    // handle err (i.e. send to Sentry) here

    return err
}

[jerbob92]

Hi @brandur,

I'd say in general I would agree, but in this case River already recovers from the panic, so in my opinion it does not make a lot of sense to not execute the rest of the middleware after reporting the panic to the error handling and return the panic as a typed error.

I will look into adding the recover in a middleware. Could you look into the incomplete context in the error/panic handler? Probably this is because context of the worker is created in the job executor, which is a different context where it reports the error. Or is this intentional? This might also be less of an issue if I would implement my own recovery handler.

[jerbob92]

I have created a middleware now that converts the panic into an error and have that passed on to the rest of the middleware, that works for us. Thanks!

[brandur]

Ah, excellent.

Rather than a generic mechanism that converts a panic an error, this strikes me as the right way to do it: if you want that behavior, add a middleware that customizes error handling to do that.

If it turns out a number of people are trying to do this, we could provide it generically. We have a rivercontrib [1] project that provides some middlewares of this sort already.

I will look into adding the recover in a middleware. Could you look into the incomplete context in the error/panic handler? Probably this is because context of the worker is created in the job executor, which is a different context where it reports the error. Or is this intentional? This might also be less of an issue if I would implement my own recovery handler.

Yeah, I thought about this for a while, but I think that the current behavior is correct. Broadly, consider that panics must be handled in either one of two places:

1. Outside (i.e. before) all middleware.
2. Inside (i.e. after) all middleware, right around where the job is going to run.

Currently River does (1). This strikes me as the better design because it means that in the event of a panic, the panic value is raised through each middleware as the stack unwinds. This is good because it gives each middleware a chance to handle the panic if it'd like to (like what yours is doing now), giving users more power to customize behavior.

However, a side effect of doing (1) is that you're deferring panic handler before running any middleware, so it's using a context set before any middleware has changed it. You might be able to override this context as you run the middleware by overwriting the context persisted above, but that seems like the wrong thing to do. If panic handling is happening outside of middleware, then it should use the context outside of middleware.

[1] https://github.com/riverqueue/rivercontrib

[jerbob92]

If it turns out a number of people are trying to do this, we could provide it generically. We have a rivercontrib [1] project that provides some middlewares of this sort already.

If it helps anyone, here is the middleware code:

var _ rivertype.WorkerMiddleware = &PanicHandler{}

type PanicHandler struct {
	river.MiddlewareDefaults
}

// captureStackTrace returns a formatted stack trace string starting after
// skipping the specified number of frames. The skip parameter should be
// adjusted so that frames you want to hide (like the ones generated by the
// tracing functions themselves) are excluded.
func captureStackTraceSkipFrames(skip int) string {
	// Allocate room for up to 100 callers; adjust as needed.
	pcs := make([]uintptr, 100)
	// Skip the specified number of frames.
	n := runtime.Callers(skip, pcs)
	frames := runtime.CallersFrames(pcs[:n])

	var stackTrace string
	for {
		frame, more := frames.Next()
		stackTrace += fmt.Sprintf("%s\n\t%s:%d\n", frame.Function, frame.File, frame.Line)
		if !more {
			break
		}
	}
	return stackTrace
}

type PanicError struct {
	Trace string
	Cause any
}

func (e *PanicError) Error() string {
	return fmt.Sprintf("PanicError: %v\n%s", e.Cause, e.Trace)
}

func (e *PanicError) Is(target error) bool {
	_, ok := target.(*PanicError)
	return ok
}

func (s *PanicHandler) Work(ctx context.Context, job *rivertype.JobRow, doInner func(context.Context) error) (err error) {
	defer func() {
		if recovery := recover(); recovery != nil {
			err = &PanicError{
				Trace: captureStackTraceSkipFrames(4),
				Cause: recovery,
			}
		}
	}()

	err = doInner(ctx)
	return err
}

If panic handling is happening outside of middleware, then it should use the context outside of middleware.

I agree, but the end user doesn't know where it's being handled. I do see now that the context value is explained in the documentation of HandleError/HandlePanic, but it could be clarified that that means you don't have access to anything the middleware might have added to the context.

[brandur]

I agree, but the end user doesn't know where it's being handled. I do see now that the context value is explained in the documentation of HandleError/HandlePanic, but it could be clarified that that means you don't have access to anything the middleware might have added to the context.

Yep good call. Opened #1074.

[bgentry]

@jerbob92 thank you so much for sharing what worked for you here! 🙏