309 handle application exceptions with 500 errors #954
Conversation
add deepseq dep Hardcode bool param for now
|
@v0d1ch I've tried out a90b671 and it doesn't seem to work properly, See https://gist.github.com/qrilka/49b0634fce17bcbd2cd4abea503a0f8c |
|
Update doesn't seem to change the result. |
|
Yeah I could probably use some help here. |
| -> RouteResult Response -> IO ResponseReceived | ||
| maybeEval resp = | ||
| if fullyEvaluate | ||
| then force resp |
There was a problem hiding this comment.
http://hackage.haskell.org/package/deepseq-1.4.3.0/docs/src/Control.DeepSeq.html#line-482 - NFData (a - > b) probably doesn't do what you expect.
servant-server/src/Servant/Server.hs
Outdated
|
|
||
| serveWithContext :: (HasServer api context) | ||
| serve :: (HasServer api '[Bool]) => Proxy api -> Server api -> Application |
There was a problem hiding this comment.
I'm against of using Context to guide whether to eval or not eval. IMHO single global choice should be enough for now.
Something #309 (comment) is good.
Context won't work for subapis anyway, as we give only single Context to serveWithContext. And if we use combinators to alter the context, we could be more direct and alter "whether to eval or not".
There was a problem hiding this comment.
The comment suggests both a global setting & a Context. Are you requesting both, or just the global?
v0d1ch
force-pushed
the
309-handle-application-exceptions-with-500-errors
branch
from
93434e4 to
5b13ff4
Compare
| routingRespond (Fail err) = respond $ responseServantErr err | ||
| routingRespond (FailFatal err) = respond $ responseServantErr err | ||
| routingRespond (Route v) = respond v | ||
| toApplication :: Bool -> RoutingApplication -> Application |
There was a problem hiding this comment.
Last nitpick: let's have
data Evaluate = Force | Lazy deriving (Show)so we won't be Bool-blind.
| @@ -46,3 +46,10 @@ instance MonadBaseControl IO Handler where | |||
|
|
|||
| runHandler :: Handler a -> IO (Either ServantErr a) | |||
| runHandler = runExceptT . runHandler' | |||
|
|
|||
| -- determins if response should be reduced to NF | |||
| data Evaluate = | |||
There was a problem hiding this comment.
@phadej Is this a good place for this type ?
| @@ -132,7 +132,7 @@ serve p = serveWithContext p EmptyContext | |||
| serveWithContext :: (HasServer api context) | |||
| => Proxy api -> Context context -> Server api -> Application | |||
| serveWithContext p context server = | |||
| toApplication (runRouter (route p context (emptyDelayed (Route server)))) | |||
| toApplication Force (runRouter (route p context (emptyDelayed (Route server)))) | |||
There was a problem hiding this comment.
@phadej Should I alter serveWithContext to take in another param that will determine if we want to NF the response or not ? How would you like this to look for the end user in a sense do you want them to be able to control the response evaluation ?
| maybeEval resp = | ||
| case fullyEvaluate of | ||
| Force -> force resp | ||
| Lazy -> resp |
There was a problem hiding this comment.
still trying to figure out what's going on here, but this looks slightly more correct to me:
| maybeEval resp = | |
| case fullyEvaluate of | |
| Force -> force resp | |
| Lazy -> resp | |
| maybeEval cont = | |
| case fullyEvaluate of | |
| Force -> \resp -> resp `deepseq` cont resp | |
| Lazy -> cont |
|
Hi @v0d1ch! Could you please rebase your PR? :) |
fixes #309