Closed snoyberg closed 7 years ago
Since we discussed this on Reddit a while ago, would you mind reviewing this @edsko?
I'm very much in favour of this PR! Big :+1: from me. Are we okay with requiring the SomeException
to be passed to the exception handler? For instance, would this preclude ResourceT
and/or Conduit
from being an instance?
(To be clear: I prefer the SomeException
to be passed, I think this is the ideal signature.)
It definitely won't preclude ResourceT
from being an instance. Your question about Conduit
is interesting... let me look into that. Right now, there's no valid definition of MonadMask
for Conduit
, but there may be some way of getting a MonadMask
instance for MonadResource m => ConduitM i o m
if we define generalBracket
correctly.
OK, I'm convinced that there's no way we'll ever get a MonadMask
for any form of ConduitM
, regardless of definition of generalBracket
. At the very least defining mask
is completely impossible.
Fair enough. That's a shame.
@RyanGlScott You have thoughts on moving forward with this?
My apologies for letting this split my mind, @snoyberg.
Overall, this looks solid. I suppose my only hesitation is what @edsko alluded to: are we absolutely certain that it's possible to define generalBracket
for every current MonadMask
instance that might exist? My gut feeling is "yes", but then again, there are likely more exotic use cases that I haven't considered (e.g., I'm not sure I understood the discussion about ConduitM
/ResourceT
).
Thanks for the review, good catches. And no problem on any delay, this is totally within normal delay timeouts (I've done far worse).
To expand a little bit on the current and proposed situation:
bracket
function existing for ConduitM
, Pipe
, or ContT
. For that matter, no valid mask
function can exist either, since we can't control the scope of the masking. So regarding these monads: both current situation and the proposed one are identical.bracket
and mask
functions. These monads include EitherT
/ExceptT
/ErrorT
(which are all isomorphic), because they can exit via a success or failure route, in addition to runtime exceptions. Unfortunately, the implementation of bracket
in the currently released exceptions
package does not recognize that, and assumes that either monadic bind will succeed or there will be a runtime exception. That's not true, and therefore bracket
for ExceptT
would break.bracket
is defined in terms of MonadMask
, and MonadMask
only requires mask
(and uninterruptibleMask
), it's easy to accidentally define a MonadMask
instance which will result in a misbehaving bracket
function. This is currently addressed in the documentation of MonadMask
.bracket
function itself (or some version of it) in the MonadMask
typeclass and allowing instances to properly deal with the extra-exit-point case, as is handled in this PR. The goal here is to define a generalized version of bracket
that allows us to define related functions like bracketOnError
.generalBracket
which will take into account the multiple exit points (both in the current transformer and all underlying transformers), and therefore each instance will need to define it directly, similar to how mask
must be defined individually.I hope that clarifies, this is certainly a confusing topic. Let me know if you have more questions.
Thanks for the explanation @snoyberg.
To be clear, my question was: are there any current MonadMask
instances that would no longer be able to be MonadMask
instances due to the introduction of generalBracket
?
Also, I'm a bit confused about the role that the "release" function (of type (a -> b -> m b)
) in generalBracket
plays in this implementation. I was expecting the "use" function (of type (a -> m b)
) to be the main exitpoint in the event that there were no exceptions, but this isn't the case here - the result of the "use" function is fed into the "release" function, and the result of the "release" function is what's returned. This is in contrast to bracket
and bracketOnError
, where in the event that there's no exceptions, the "release" function is only ran for side effects, and the result of the "use" function is what's returned.
I'm guessing this is another generality that generalBracket
provides over bracket
/bracketOnError
. If so, it would be nice to note this in the documentation, as it currently suggests that generalBracket
simply grants the ability to distinguish between exit cases, when in actuality it's more subtle than that.
I'd like to be able to give you a guarantee that no current MonadMask
instances will be broken, but I can't prove that. I can say that I can't think of any such cases, and that (as you've already seen) I've worked through the instances present in this package.
Allowing the release function to modify the result is just a generalization. I can document that, or remove the functionality, whichever you prefer. I didn't actually consider it an important part of the design here.
As regards the type of the release function: I like that it tells the story very clearly in the type; it can only be called if the computation was successful (input) and in fact must be called (otherwise the result would not match). It's a nice spec.
Just general thoughts from my first skim: looks good!
I'll do a more detailed pass soon.
Let's merge this and fix it up in the main branch.
Thanks!
On Thu, Jun 29, 2017 at 3:02 PM, Edward Kmett notifications@github.com wrote:
Merged #59 https://github.com/ekmett/exceptions/pull/59.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ekmett/exceptions/pull/59#event-1144069959, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBB426MMeQJKAyK6WyQse74DjXMYcPks5sI5I7gaJpZM4NxKdo .
This allows for a
MonadMask
instance forExceptT
andErrorT
, and generally makes it harder to accidentally make incorrectMonadMask
instances for transformers.