Make catching exceptions configurable.

[jappeace]

Fixes #1771

This is done by adding a function to Yesod
typeclass which can match on any exception
and tell the framework if it should rethrow
or not.

I used an overridable function because it seemed
more flexible then a whitelist.
A user can now for example choose to throw
everything, or catch everything as easily.

---

Before submitting your PR, check that you've:

• Bumped the version number
• Documented new APIs with Haddock markup
• Added @since declarations to the Haddocks for new, public APIs

After submitting your PR:

• Update the Changelog.md file with a link to your PR
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

[snoyberg]

@pbrisbin would you be able to look this over and see if it addresses your needs?

[snoyberg]

This may be too limiting an API. We've often run into situations later where people say they want to be able to change behavior at runtime. What do you think of something like:

catchBehavior :: site -> SomeException -> IO CatchBehavior

[snoyberg]

Also, we may want to change the default behavior back to what it used to be (not catching async exceptions).

[jappeace]

Hmm, perhaps it's still sensible to handle most exceptions, even async ones in yesod. For example if a thread is killed or an allocation limit reached, my expectation would be that the error page is rendered by yesod, not by warp.

[snoyberg]

@pbrisbin thoughts?

[pbrisbin]

Hmm. Well this is tricky because I'm coming at it from a couple of different perspectives.

As a user of Yesod on behalf of my users, I'd agree that I'd prefer if the normal error handling and error page triggered on ThreadKilled and AllocationLimitExceeded. Because, why not?

As an operator of Yesod, I'm not sure if I'd hold that preference for "most async exceptions", because I think there are a lot of async exceptions that are normal in the context of handling web requests, don't ever reach or impact end users, and I don't want them inflating error metrics as if they had.

As someone familiar with the implementation inolved, I don't agree that I'd expect the normal error handling and error page triggered on these two exceptions in particular. In my head, the scope of handling a web request is

Warp
  Wai
   |----------|
   | Yesod    |
   |   My App |
   |----------|

In the case of these two exceptions in particular, I'd expect everything I've boxed there to be dead, like kill -9 dead. And so I personally wouldn't expect any error recovery to be possible at that level for such exceptions.

Knowing what I've learned recently about async exceptions, I think I could safely apply this expectation to "most async exceptions". I think they all represent kill -9-like messaging and so there should be no recovery.

[jappeace]

so I guess for most users this default of catching everything except timeout & ConnectionClosedByPeer constructor is sensible, but as an operator you could still override this behavior? In other words, everyone is happy?

[pbrisbin]

As long as we end up in a place where TimeoutError behaves correctly, I'm happy. If we also end up in a place where it's easy for me to say "don't catch any async exceptions", I'm very happy.

I think "don't catch async exceptions" is a more sensible default, for the reasons I tried to describe above, but it's subjective and I don't mind if you go the other way as you're writing the code and Michael can choose to merge it. I just don't see either of us convincing the other, but I don't think we need to.

[jappeace]

I added a test to verify by default we don't catch Timeout either: https://github.com/yesodweb/yesod/pull/1772/files#diff-bd8750d9a236376b742e82438d8e92aeeb76f1d55827fc85c21154c5c4e8f9a0R378

I think that's what you meant, right?

There is a rethrow async function as well (although not as well tested): https://github.com/yesodweb/yesod/pull/1772/files#diff-1fdc39d8a397f906d8f4f69a087048f3432df0b1fd2238ee5459ff1e89bb0edfR655

[snoyberg]

I think "don't catch async exceptions" is a more sensible default

I think I lean towards this way of thinking, I was already nervous about the original approach since it violates what I generally recommend for async exception handling. I'd be tempted to call the previous change in behavior a bug (since it silently broke other people's code) and go back to the original default, still exposing this configuration point.

To fully flesh out the thinking here, I see four points in the design space for defaults:

1. Rethrow all async exceptions (original behavior)
2. Rethrow a specific list of async exceptions (current PR)
3. Catch a specific list of async exceptions (not proposed yet)
4. Catch all async exceptions (current behavior)

I'm not convinced (2) is a good compromise point, since including Timeout may seem like a good idea here, but honestly seems arbitrary to me. It's entirely possible there's someone else out there using a different async exception for middlewares that we're not aware of.

[jappeace]

both choices are arbitrary, in anycase I'll rewrite it to the 'rethrow async exception' behavior as default. having this configurable is good enough for me.

[snoyberg]

I'm not sure what other behaviors may be relevant in the future, but we may want other possibilities than just rethrow and catch (e.g., "wrap up with this function"). Providing an abstract type and smart constructors would allow for future extensibility.

[pbrisbin]

I don't have very strong opinions on the implementation, this seems to offer a way for me to say "don't catch async exceptions", which would be great.

I do have some questions, for my own curiosity:

Is there really value in restricting the possibilities to the two-constructor CatchBehavior enumeration, rather than some fully open SomeException -> m () that the user could make do whatever they want?

I see "(even async), except for the 'Warp.ConnectionClosedByPeer' constructor", and I would humbly suggest TimeoutError be included in any such lists, as that was a very clear exception not to catch no matter what.

I'm a little curious how this approach compares to using the existing yesodMiddleware hook. That strikes me as an existing hook that is a perfectly reasonable place to define this behavior.

For example, if we want the default behavior to not catch, a user could add catching by:

instance Yesod MyApp where
  yesodMiddleware =  alsoCatchAsyncMiddleware . defaultYesodMiddleware

alsoCatchAsyncMiddleware h = h `EUnsafe.catch` \ex ->
    errorHandler $InternalError $ pack $ displayException ex

Or if we want the default behavior to be to catch, a user could get not catching by:

instance Yesod MyApp where
  yesodMiddleware =  defaultYesodMiddlewareNoAsyncCatch

defaultYesodMiddleware =  alsoCatchAsyncMiddleware . defaultYesodMiddlewareNoAsyncCatch

All naming TBD of course.

Is there something about that that doesn't work for the "please catch async and render error page" or "please don't catch async" use-cases?

[jappeace]

@pbrisbin

rather than some fully open SomeException -> m () that the user could make do whatever they want?

how would this work then? if the callback doesn't throw an exception we should let yesod do the error rendering? It's not very obvious to me from just the type signature.

and I would humbly suggest TimeoutError be included in any such lists, as that was a very clear exception not to catch no matter what.

Yes this is a good idea. Sorry I misunderstood why at first but now I realize the timeout function is supposed to catch this.

That strikes me as an existing hook that is a perfectly reasonable place to define this behavior.

This would work, but I think by default yesod should handle exceptions properly, that is to say render those with a 500 screen within yesod, except when the user doesn't want this to happen, which is why I also agree it should be configurable.

[pbrisbin]

Everything's good from my end, but just to reply to your questions:

how would this work then? if the callback doesn't throw an exception we should let yesod do the error rendering? It's not very obvious to me from just the type signature.

Yeah I was being vague, because I don't know exactly how it might work. I just know that your original PR (basically) replaced an UnliftIO.catch with an EUnsafe.catch, so I thought an initial stab at configurability would be to inject exactly that:

class Yesod where
  -- | By default we catch all exceptions to an error page, replace this with
  -- 'UnliftIO.Exceptions.catch' to only catch synchronous exceptions
  catch = EUnsafe.catch

But I know this probably won't work because Yesod(catch) has to be at some m that both of those could satisfy.

This would work, but I think by default yesod should handle exceptions properly

Yeah, my example showed it with either default behavior. I was just curious if I was missing something that meant using yesodMiddleware to add exception-handling wouldn't function, but it sounds like it that's not the case.

[snoyberg]

But I know this probably won't work because Yesod(catch) has to be at some m that both of those could satisfy.

Given the fact that everything involved is a MonadUnliftIO (I think), specializing this to IO should work, and the internals of Yesod can wrap it up using withRunInIO. In other words, a new method along these lines may solve the problem:

catchHandlerExceptions :: site -> IO a -> (SomeException -> IO a) -> IO a

[jappeace]

I got rid of catchbehavior and now use the provided catch function as suggested.
The real change ends up being that you now can rethrow any exception you like (async and sync),
as is shown by CustomApp.

I removed all the code and tests involved with pattern matching on the ConnectionClosedByPeer constructor and timeout type.

[pbrisbin]

I really like this implementation! Thanks for working through it.

[snoyberg]

LGTM, thanks!