Carry the exception

[parsonsmatt]

This PR amends ReleaseException to carry the exception that was thrown.

This is a pretty big breaking change unfortunately - lots of instances are removed. I can probably recover some of them, especially if I use Maybe SomeException and just dump Nothing in for the bad ones.

Probably only useful for debugging.

Fixes #460

[snoyberg]

Looks like a sensible change, and I'd be OK with the breakage. This should likely come along with a major version bump and changelog entry explaining the change. Ignoring PVP noise, I'm guessing this won't actually break much code in the wild.

Did you want to do more testing with this branch before we make a release?

[shlevy]

@parsonsmatt Still hoping to take this, or can I take it over? Came looking for exactly this.

[shlevy]

FWIW this is useful for more than debugging, unless you include detailed instrumentation in prod "debugging". I want to have resource-scoped events that store exception information when they exit unsuccessfully.

[shlevy]

Instead of Maybe SomeException we want ReleaseAbort like MonadMask?

[shlevy]

Is this expected to be right? 😬

[parsonsmatt]

I've used it a bunch and it uhhh does mostly work

[mbj]

Until future changes to GHC render the backtrace in show?

[parsonsmatt]

No, those would only be able to work on the show :: SomeException -> String type, since SomeException carries the implicit parameters for the stack trace. This uses the Show a that is implied by the Exception a dictionary that is packed in the GADT.

[parsonsmatt]

I think I was able to use this to debug the issue in persistent successfully, but I don't think we continued using it further. Happy to pass it along.

[shlevy]

@snoyberg Still open to this? OK at add ReleaseAbort for cases where we can't get the SomeException like MonadMask does?

[snoyberg]

Making weird carve outs for MonadMask seems like a bad call to be honest. I'm overall hesitant to make a change like that if it will mean significant ecosystem breakage.

[shlevy]

It's not a carve-out for MonadMask, it's a case that exists in MonadMask but one we might have to replicate here. That is, if we're in a MonadResource which in some context is able to tell that a resource is being released due to error but doesn't have the Exception, then it would be good to have a special ReleaseAbort case much like MonadMask has ExitCaseAbort. I don't know if such cases exist, but @parsonsmatt's difficulty in generating some instances suggests they do. Alternatives are:

1. Status quo, no exception information on erroneous release, resourcet can't be used for instrumentation that captures exception information on failure
2. Add a SomeException field to ReleaseException. That's what this PR does, possibly requires losing some instances
3. Add a Maybe SomeException field to ReleaseException, with Nothing for cases where we can't (or it's too complicated to) find the exception. That's @parsonsmatt's suggestion
4. Add a SomeException to ReleaseException and add a ReleaseAbort case. This is isomorphic to 3, except that it will be slightly more idiomatic to those used to MonadMask.

I'm happy to finish up any of 2-4 if they'd be welcome.

[snoyberg]

I’m very much on the fence on this. I understand the motivation for this. However, adding a breaking change to a long-stable library when there are still a few open design questions is a bigger deal than I initially recognized.

Are there no ways to achieve the same thing you’re doing here with the current API?

[shlevy]

My use case is a generic event-oriented instrumentation library. My typical usage is the withX pattern, where I can straightforwardly capture exception information in the instrumentation: https://hackage.haskell.org/package/eventuo11y-0.1.0.1/docs/Observe-Event.html#g:resourcesafe

But resourcet exists for a reason, sometimes scope-based deallocation is insufficient, so I have Acquire variants as well: https://hackage.haskell.org/package/eventuo11y-0.1.0.1/docs/Observe-Event.html#g:3. And I don't see any way to reliably capture exception information without going back to a scope-based approach, I have to set up a catch somewhere.

For a less intrusive option, perhaps we can add a way to add a Maybe SomeException -> IO () handler to a ReleaseKey, with a guarantee that it will be called before the release function if we're in a ReleaseException state?

[snoyberg]

OK, I've given it some thought, and the change does seem reasonable. I want to propose a slight modification to (4) above: would it be possible to keep the current data constructors unmodified and add a new data constructor that captures the SomeException? It seems like this would have the smallest impact on existing users.

It could also be that I'm being overly worried, and the pattern synonyms included here do the heavy lifting for us. I'm simply inexperienced with using pattern synonyms at all.

For me, the guiding principle is: let's do this in a way that minimizes impact on end users as much as possible.

[shlevy]

@snoyberg I'm not sure if this can be done with clever use of pattern synonyms, but if we're trying to minimize impact then I think we'd want (modulo naming bikeshedding):

1. Code which constructs using ReleaseException instead constructs ReleaseUnknownException
2. Code which matches on ReleaseException instead matches on ReleaseUnknownException and ReleaseKnownException _

If 2 isn't quite possible as described, then probably we can do:

1. Code which constructs using ReleaseException instead constructs ReleaseExceptionReal Nothing
2. Code which matches on ReleaseException instead matches on ReleaseExceptionReal _

I can look into whether either of these is possible, if you like the approach

[snoyberg]

No strong opinion from me, I noticed the pattern synonym in this existing PR, but I really don't understand the impact on backwards compat.

[parsonsmatt]

I don't think ExitCaseAbort from MonadMask makes much sense here. The definition is there to support ExceptT and friends, but this library makes use of MonadUnliftIO which cannot support them.

For backwards compatibility, the current code allows you to continue pattern matching on ReleaseException, but any construction will error (since the constructor is gone). With the addition of a {-# COMPLETE #-} pragma, then end users won't even get a warning when pattern matching on ReleaseNormal | ReleaseEarly | ReleaseException.

In hindsight, I'm not really in favor of having a Maybe SomeException - if it failed due to an exception, how can you not have the exception? Allowing Nothing here just lets folks defeat the entire purpose of carrying a problem at all, and ReleaseEarly should be fine.

[shlevy]

@parsonsmatt Ah, I hadn't looked at the code but I assumed there were instances where you legitimately couldn't get the exception. If that's not the case then yeah we should keep the code as it is. What did you mean by:

I can probably recover some of them, especially if I use Maybe SomeException and just dump Nothing in for the bad ones.

?

[parsonsmatt]

I can probably recover some of them, especially if I use Maybe SomeException and just dump Nothing in for the bad ones.

I am not sure what I was thinking. It looks like we lost Read, Ord, Enum, Bounded. None of which seem super useful TBH. Like I doubt anyone is doing if releaseType < ReleaseException then .... Or trying to store them in a Set, or enumerating the possible constructors.

[shlevy]

Ah, instances for ReleaseType! I assumed this was instances of MonadResource or something. Cool, OK, will open an updated PR.

[shlevy]

@parsonsmatt Let me know if you want me to remove your Co-authored-by on #498