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

more general/abstract error handling #45

Open
hasufell opened this issue Apr 30, 2016 · 9 comments
Open

more general/abstract error handling #45

hasufell opened this issue Apr 30, 2016 · 9 comments

Comments

@hasufell
Copy link

hasufell commented Apr 30, 2016

I expected something like this to work:

test = run $ (fetch $ select [] "ThisDoesNotExist") <|> (fetch $ select [] "thisExists")

Instead, I just get an Exception. Using findOne doesn't work either, since it's always a success.

IMO, the exception should only happen if the last alternative fails. At least that's the expected semantics of (<|>).

@hasufell hasufell changed the title can't use Alternative instance for Action type? can't use Alternative instance for Action/ReaderT type? Apr 30, 2016
@VictorDenisov
Copy link
Member

Where did you find this semantics of Alternative? The only meaning of <|> - it's a binary associative operation on applicative functors. Look at the implementation of alternative for ReaderT. It only lifts whatever underlying monad implements. There is no Alternative for IO.

@hasufell
Copy link
Author

Where did you find this semantics of Alternative?

Uhm, that's basically how people use this, including the most famous Parser libraries. And it's also the reason this class is called Alternative. It's a "success selector" if you will.

Look at the implementation of alternative for ReaderT. It only lifts whatever underlying monad implements. There is no Alternative for IO.

Yes, the problem is the error handling which is done via MonadIO:

fetch :: (MonadIO m) => Query -> Action m Document
fetch q = findOne q >>= maybe (liftIO $ throwIO $ DocNotFound $ selection q) return

I am sure this could be done more general than invoking throwIO inside the Action context.

@VictorDenisov
Copy link
Member

If you want a better error handling, I agree, there is a lot of room for improvement in this area, but it's not about Alternative. In most of the cases your <|> won't work because in most of the cases m is IO and IO is not an instance of Alternative. So, you should avoid using <|> in any case.
This code doesn't work either, even though nobody throws exceptions:

v <- access p master "admin" (serverVersion <|> serverVersion)

@hasufell
Copy link
Author

hasufell commented Apr 30, 2016

There is no Alternative for IO.

But for MonadThrow.

I did some testing and got weird results, e.g. the following fetch function does what I want:

fetch :: (MonadIO m, MonadThrow m) => Query -> Action m Document
fetch q = findOne q >>= maybe (throwM $ userError "") return

However, trying that with our own Exception type does not work, and there's no indication why, since the types seem to match:

fetch :: (MonadIO m, MonadThrow m) => Query -> Action m Document
fetch q = findOne q >>= maybe (throwM $ DocNotFound $ selection q) return

Or simpler, in ghci:

*Database.MongoDB Control.Monad.Catch Control.Applicative> (throwM $ userError "blah") <|> (print 3)
3

*Database.MongoDB Control.Monad.Catch Control.Applicative> (throwM $ AggregateFailure  "blah") <|> (print 3) 
*** Exception: AggregateFailure "blah"

It's not clear to me where the difference comes from. It seems userError triggers different behavior, although the types are the same more or less.

@VictorDenisov
Copy link
Member

I suggest to rename this issue to something link: introduce a more general error reporting to the driver.

@hasufell hasufell changed the title can't use Alternative instance for Action/ReaderT type? more general/abstract error handling Apr 30, 2016
@hasufell
Copy link
Author

@hasufell
Copy link
Author

The reason is https://hackage.haskell.org/package/transformers-0.5.2.0/docs/src/Control.Monad.Trans.Error.html
more specifically:

instance MonadPlus IO where
    mzero       = ioError (userError "mzero")
    m `mplus` n = m `catchIOError` \ _ -> n

So Alternative uses (<|>) = mplus and that ends up with the above definition for userError which catches the error.

@hasufell
Copy link
Author

hasufell commented Apr 30, 2016

After a lengthy discussion in #haskell people were suggesting to use a custom infix operator instead of (<|>). I guess that would make sense. Maybe something like tryAction?

Edit: even that seems complicated, because the current error handling is a wild mixture of IOException type and the custom Failure type, so when you want to match/catch against the custom exception type via MonadError Failure m you get type errors like:

    Couldn't match type ‘GHC.IO.Exception.IOException’ with ‘Failure’
    arising from a functional dependency between:
      constraint ‘Control.Monad.Error.Class.MonadError Failure IO’
        arising from a use of ‘tryAction’
      instance ‘Control.Monad.Error.Class.MonadError
                  GHC.IO.Exception.IOException IO’

@kaol
Copy link

kaol commented May 9, 2018

I ran into another manifestation of this.

look "a" [] <|> look "b" [] :: Maybe Value yields Nothing but

look "a" [] <|> look "b" [] :: Either String Value throws an exception.

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

No branches or pull requests

3 participants