Communicate #after hook errors in Middleware

[rngtng]

As of #1240 the #after hook is called on middlewares even when exception is raised. This breaks middleware which assume response, (@app_response) is set. (e.g. grape_logging).

Firstly, any middleware should be migrated to handle this case, secondly grape should give a warning at least, to help debugging and prevent the application from hard failure.

See discussion at: #1240 (comment)

[dblock]

👍

[jpaas]

I am also running into this issue. My logging middleware is unable to log the response in the after hook.

[gregormelhorn]

I am currently trying to fix this issue. One thing I stumbled upon is that the formatter also uses error handling to communicate issues in the after callback.

Fortunately, from what I can see now, catch(:error) and rethrowing while swallowing any StandardError from the after callback would fix the issue - but it also alters the behaviour of the formatter. I am not sure if this should really be implemented I have to say.

I would vote for a setting to switch on and off the "rescue StandardError behaviour" in the after callback (while still keeping and rethrowing any explicitly thrown error).

What do you think?

[gregormelhorn]

The more I think about it, the more it would make sense to add a backwards compatibility switch (do not run after callbacks on error) instead of magically catching exceptions...

[dblock]

@gregormelhorn Maybe. That seems to undo the change we made, I'd rather not have both behaviors and issue a warning as described in this issue. But do try some code and show us what happens before/after.

[gregormelhorn]

The changes mean that any StandardError thrown gets transformed into a warning.

It also means that catching standard exceptions inside formatters is not possible, so I removed the corresponding tests. After all, the code does not care if the issue is in the after callback of a logging middleware or the after callback of a formatter.

I am still not sure if making this fail hard with exceptions is not the better way - it will be very easy to miss coding errors.

[rngtng]

Ok, true a hard fail makes sense, but for debugging reason It would help a lot to know that the error happened in middleware so what about a warning + re-raise the original error?

[gregormelhorn]

ok, I changed it accordingly 👍

[dblock]

Closed via #1285.

[rngtng]

nice, thx