Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: allow catching of errors thrown from IO #104

Closed
patrickt opened this issue Jan 27, 2019 · 7 comments
Closed

Feature request: allow catching of errors thrown from IO #104

patrickt opened this issue Jan 27, 2019 · 7 comments
Labels
enhancement New feature or request

Comments

@patrickt
Copy link
Collaborator

patrickt commented Jan 27, 2019

Right now, it's not necessarily clear that Control.Effect.Error can't capture errors that happen in IO. The following code looks like it should return a Left SomeException value, since it runs with Error SomeException as a constraint, but if we throw an error inside a Lift IO effect the exception is not caught:

runM . runError @_ @_ @Exc.SomeException $ (() <$ liftIO (ioError (userError "boom")))
-- *** Exception: user error (boom)

I have a branch where I add a catchIO function that handles this properly, using the Resource effect to unlift Control.Exception.catch to an effectful context:

catchIO :: (Member Resource sig, Carrier sig m)
        => m a
        -> (forall e . Exc.Exception e => e -> m a)
        -> m a

runM . runResource runM $ catchIO (True <$ liftIO (ioError (userError "boom"))) (const (pure False))
-- False

I am currently working inside an effect stack that delegates often to IO actions, and every time I call liftIO I open myself to the possibility of unhandled exceptions and early termination of my program, so this functionality is a big win to me. I can't see much downside to adding this, though it's arguable that it deserves its own effect rather than being foisted into Resource, and that catchIO is not the best name. I don't think catch is appropriate, given that we already provide catchError.

@patrickt patrickt added the enhancement New feature or request label Jan 27, 2019
@patrickt
Copy link
Collaborator Author

Maybe catchException? But does that imply the existence of a throwException, which I'd argue that we shouldn't implement?

@robrix
Copy link
Contributor

robrix commented Jan 28, 2019

IIRC, it’s possible to do this with just Lift IO. I’ll try to find a reference.

@patrickt
Copy link
Collaborator Author

patrickt commented Jan 29, 2019

@robrix I don't think it is; the signature of catch is

Exception e => IO a -> (e -> IO a) -> IO a

You can call liftIO (catch f h), of course, but that constrains the block and handler to run in IO, which is unnecessarily restrictive. Would love to be wrong though, especially if Lift IO gives us an unlift operation.

@robrix
Copy link
Contributor

robrix commented Jan 29, 2019

This is what we’re doing elsewhere, I believe this was originally based on Oleg’s work:

-- | Generalize 'Exc.catch' to other 'MonadIO' contexts for the handler and result.
catchIO :: ( Exc.Exception exc
           , MonadIO m
           )
        => IO a
        -> (exc -> m a)
        -> m a
catchIO m handler = liftIO (Exc.try m) >>= either handler pure

@patrickt
Copy link
Collaborator Author

patrickt commented Jan 29, 2019

What I'd prefer is

catchIO :: ( Exc.Exception exc
           , Member CatchIO sig
           , Carrier sig m, MonadIO m
           )
        => m a
        -> (exc -> m a)
        -> m a

I can write a new effect in our downstream code to do this, if you'd prefer to keep this out of fused-effects proper - either way is fine with me. Thoughts? (If we elect not to do this, we should make a note in Control.Effect.Error alerting that IOExceptions thrown from IO are not catchable, and that the above pattern with try is to be preferred.)

@robrix
Copy link
Contributor

robrix commented Jan 29, 2019

This might suggest that Resource and this could both be reduced to a RunIO effect providing essentially m a -> IO a.

@patrickt
Copy link
Collaborator Author

Retiring this issue in favor of a general RunIO effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants