Catch ServantErr exceptions in liftIO definition for Handler?

[dbaynard]

Would it be possible to change the MonadIO instance for Handler to the following (lawful) instance?

instance MonadIO Handler where
  liftIO :: IO a -> Handler a
  liftIO = Handler . ExceptT . tryJust (fromException @ServantErr)

The original is here:

servant/servant-server/src/Servant/Server/Internal/Handler.hs

Lines 28 to 33 in 109f7b2

	newtype Handler a = Handler { runHandler' :: ExceptT ServantErr IO a }
	deriving
	( Functor, Applicative, Monad, MonadIO, Generic
	, MonadError ServantErr
	, MonadThrow, MonadCatch, MonadMask
	)

This ensures that all ServantErr exceptions are handled.

As far as I can tell, there are two basic ways to throw a ServantErr.

An example (with output)

{-# LANGUAGE DataKinds        #-}
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE TypeOperators    #-}

module Main where

import           Control.Monad.Catch      (throwM)
import           Network.Wai              (pathInfo)
import           Network.Wai.Handler.Warp (Settings, defaultSettings,
                                           runSettings, setLogger, setPort)
import           Servant

main :: IO ()
main = runSettings settings app

settings :: Settings
settings = setPort 8080
  . setLogger (\req st _mSize -> print st *> print (pathInfo req))
  $ defaultSettings

app :: Application
app = serve @API Proxy server

type API
    = "throwError" :> Get '[JSON] Int
 :<|> "throwM" :> Get '[JSON] Int
 :<|> "pure" :> Get '[JSON] Int

server :: Server API
server = throwError err400
 :<|> throwM err400
 :<|> pure 200

λ> :main
Status {statusCode = 400, statusMessage = "Bad Request"}
["throwError"]
Status {statusCode = 500, statusMessage = "Internal Server Error"}
["throwM"]
ServantErr {errHTTPCode = 400, errReasonPhrase = "Bad Request", errBody = "", errHeaders = []}
Status {statusCode = 200, statusMessage = "OK"}
["pure"]
Status {statusCode = 404, statusMessage = "Not Found"}
["whoops"]

throwError :: ServantErr -> Handler a
throwM :: ServantErr -> Handler a

The former is considered an application error, and will be processed by servant. The latter will not, and will be processed by the server (in this case, warp). I can't see any circumstances in which anybody may wish to throw a server error (throwM), as opposed to an application error (throwError). Briefly,

λ> runHandler (throwError err400)
Left (ServantErr {errHTTPCode = 400, errReasonPhrase = "Bad Request", errBody = "", errHeaders = []})
λ> runHandler (throwM err400)
*** Exception: ServantErr {errHTTPCode = 400, errReasonPhrase = "Bad Request", errBody = "", errHeaders = []}

In using a custom handler, I've encountered an issue resulting from this, which I've realised might be best fixed by changing the MonadIO instance for Handler.

If there's no reason to throw a server error, then would it not be better to define the instance so that any ServantErr is processed as an application error instead?

[phadej]

Use application specific monad; not Handler. Then you have freedom to make whatever you want regarding exceptions. My priority is to drive #841 home, so we could remove ExceptT out of Handler. As that removal is a goal of design, catching ServantErr doesn't make sense long-term. Use application specific monad and `hoistServer`.

…

On 3 Feb 2019, at 15.01, David Baynard ***@***.***> wrote: Would it be possible to change the MonadIO instance for Handler to the following (lawful) instance? instance MonadIO Handler where liftIO :: IO a -> Handler a liftIO = Handler . ExceptT . tryJust (fromException @ServantErr) The original is here: https://github.com/haskell-servant/servant/blob/109f7b2a45ffe86dd707b89bf5fd51d440381b05/servant-server/src/Servant/Server/Internal/Handler.hs#L28-L33 This ensures that all ServantErr exceptions are handled. As far as I can tell, there are two basic ways to throw a ServantErr. An example (with output) throwError :: ServantErr -> Handler a throwM :: ServantErr -> Handler a The former is considered an application error, and will be processed by servant. The latter will not, and will be processed by the server (in this case, warp). I can't see any circumstances in which anybody may wish to throw a server error (throwM), as opposed to an application error (throwError). Briefly, λ> runHandler (throwError err400) Left (ServantErr {errHTTPCode = 400, errReasonPhrase = "Bad Request", errBody = "", errHeaders = []}) λ> runHandler (throwM err400) *** Exception: ServantErr {errHTTPCode = 400, errReasonPhrase = "Bad Request", errBody = "", errHeaders = []} In using a custom handler, I've encountered an issue resulting from this, which I've realised might be best fixed by changing the MonadIO instance for Handler. If there's no reason to throw a server error, then would it not be better to define the instance so that any ServantErr is processed as an application error instead? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.