309 handle application exceptions with 500 errors by v0d1ch · Pull Request #954 · haskell-servant/servant

v0d1ch · 2018-04-26T13:52:37Z

fixes #309

add deepseq dep Hardcode bool param for now

qrilka · 2018-04-27T08:43:34Z

@v0d1ch I've tried out a90b671 and it doesn't seem to work properly, See https://gist.github.com/qrilka/49b0634fce17bcbd2cd4abea503a0f8c

qrilka · 2018-04-27T09:16:42Z

Update doesn't seem to change the result.
BTW maybe it's worth to fix issue with boolean blindness here?
Using serveWithContext and opaque (True :. EmptyContext) doesn't look quite user-friendly API

v0d1ch · 2018-04-27T09:35:12Z

Yeah I could probably use some help here.

phadej

Test case would be also nice. (Something which was broken before, but isn't broken with the patch)

phadej · 2018-04-29T14:26:46Z

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

+              -> RouteResult Response -> IO ResponseReceived
+    maybeEval resp =
+      if fullyEvaluate
+        then force resp


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.

phadej · 2018-04-29T14:28:24Z

servant-server/src/Servant/Server.hs

-serve p = serveWithContext p EmptyContext

-serveWithContext :: (HasServer api context)
+serve :: (HasServer api '[Bool]) => Proxy api -> Server api -> Application


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".

The comment suggests both a global setting & a Context. Are you requesting both, or just the global?

Just global I think.

…-errors

phadej · 2018-07-09T20:30:29Z

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

-  routingRespond (Fail err)      = respond $ responseServantErr err
-  routingRespond (FailFatal err) = respond $ responseServantErr err
-  routingRespond (Route v)       = respond v
+toApplication :: Bool -> RoutingApplication -> Application


Last nitpick: let's have

data Evaluate = Force | Lazy deriving (Show)

so we won't be Bool-blind.

v0d1ch · 2018-07-09T21:36:49Z

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

 runHandler = runExceptT . runHandler'
+
+-- determins if response should be reduced to NF
+data Evaluate =


@phadej Is this a good place for this type ?

v0d1ch · 2018-07-09T21:40:19Z

servant-server/src/Servant/Server.hs

    => 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))))


@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 ?

fisx · 2020-06-13T19:59:48Z

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

+    maybeEval resp =
+      case fullyEvaluate of
+        Force -> force resp
+        Lazy  -> resp


still trying to figure out what's going on here, but this looks slightly more correct to me:

Suggested change

maybeEval resp =

case fullyEvaluate of

Force -> force resp

Lazy -> resp

maybeEval cont =

case fullyEvaluate of

Force -> \resp -> resp `deepseq` cont resp

Lazy -> cont

tchoutri · 2021-11-17T14:02:15Z

Hi @v0d1ch! Could you please rebase your PR? :)
If you are willing to finish the work on this, it can be feature in the next major release of Servant!

Sasa Bogicevic added 3 commits April 26, 2018 14:24

Remove duplicate import

ff4e2af

Add NFData instance

547adab

add deepseq dep Hardcode bool param for now

Fix all errors

a90b671

Remove hardcoded param in serveWithContext

4d46e98

Add helper functions

3d9a151

phadej suggested changes Apr 29, 2018

View reviewed changes

Sasa Bogicevic added 2 commits June 10, 2018 18:35

Merge branch 'master' into 309-handle-application-exceptions-with-500…

64686f3

…-errors

Merge branch 'master' into 309-handle-application-exceptions-with-500…

5b13ff4

…-errors

v0d1ch force-pushed the 309-handle-application-exceptions-with-500-errors branch from 93434e4 to 5b13ff4 Compare July 5, 2018 22:30

Make everyting compile again

465e0b8

phadej reviewed Jul 9, 2018

View reviewed changes

Use Evaluate instead of Bool

a786dc4

v0d1ch commented Jul 9, 2018

View reviewed changes

symbiont-joseph-kachmar mentioned this pull request Jul 3, 2019

Proper servant-server exception handling for warp/wai #1192

Open

fisx reviewed Jun 13, 2020

View reviewed changes

tchoutri added the 🚧 needs rebase 🚧 label Nov 17, 2021

Uh oh!

Conversation

v0d1ch commented Apr 26, 2018

Uh oh!

qrilka commented Apr 27, 2018

Uh oh!

qrilka commented Apr 27, 2018

Uh oh!

v0d1ch commented Apr 27, 2018

Uh oh!

phadej left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchoutri commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

phadej left a comment •

edited

Loading