Add a field to Delayed for storing a clean up action #622

alpmestan · 2016-10-21T10:37:21Z

I need something like this in servant-multipart in order to delete files created in a temporary dir once the handler has run.

This is not the definitive code, but at least we can start a discussion.

For instance, @jkarni pointed out that the clean up might not always run with the code as is because of exceptions.

I decided to make the clean up actions live in IO (and not DelayedIO) because most of the time, if not always, the code we want to run there lives in IO and doesn't really need access to the request, but rather to something created in the body.

jkarni

I think it's also pretty crucial to have tests for (a) the basic functionality, and (b) verifying that the cleanup action is run when exceptions are thrown in a range of situations.

jkarni · 2016-10-21T13:20:07Z

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

@@ -398,7 +398,7 @@ instance HasServer Raw context where
  type ServerT Raw m = Application

  route Proxy _ rawApplication = RawRouter $ \ env request respond -> do
-    r <- runDelayed rawApplication env request
+    (r, _) <- runDelayed rawApplication env request


Why isn't the cleanup run in Raw?

Oops, I just made it typecheck, not correct =) Should it be done by doing (case ...)finallycleanup to mimick the behavior we'll introduce in the runAction function?

jkarni · 2016-10-21T14:13:16Z

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

+runAction action env req respond k = do
+  (routeResult, cleanup) <- runDelayed action env req
+  resp <- go routeResult
+  cleanup


I think this should be bracket (runDelayed action env eq) snd (go . fst)?

It should indeed.

Modulo a call to respond.

jkarni · 2016-10-21T14:17:28Z

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

@@ -103,6 +105,10 @@ data Delayed env c where
             , authD     :: DelayedIO auth
             , bodyD     :: DelayedIO body
             , serverD   :: captures -> auth -> body -> Request -> RouteResult c
+             , cleanupD  :: body -> IO ()


As I mentioned elsewhere, I don't know exactly why you need the body arg. Or why just the body, for that matter. DelayedIO (IO ()) seems like a more intuitive signature, to me.

1/ Why is it more intuitive? Clean up will rarely (if ever) need to look at the raw request or perform routing.

2/ In my case, I get filepaths of temp files and what not by looking at body. We can indeed add other arguments to cleanupD, but not make it a simple action, because I otherwise don't have any mean for my cleanup thing to get its hands on the temp files it is supposed to delete. More generally, this feature is useful anytime we want to clean up after body checks and other stuffs (anything, really). Except maybe for somewhat "global" stuffs, we need a way to for the clean up action to get its hands on whatever it is that needs cleaning up, so I use an argument for that.

I would not mind adding more arguments (results of the other checks). But making it a DelayedIO so that I have to parse everything from the request again? No.

alpmestan · 2016-10-21T17:10:39Z

Yes, I'll write tests before it's approved for merging.

soenkehahn

Generally I think this seems like a good thing to have.

I would also welcome a test that illustrates your exact use-case. So that we can more freely think about how to solve the problem in the simplest way.

soenkehahn · 2016-10-23T15:16:13Z

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

-    , serverD = \ c a (z, v) req -> ($ v) <$> serverD c a z req
+    { bodyD    = (,) <$> bodyD <*> new
+    , serverD  = \ c a (z, v) req -> ($ v) <$> serverD c a z req
+    , cleanupD = cleanupD . fst -- not sure that's right


Seems right to me. :)

soenkehahn · 2016-10-23T15:17:12Z

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

+        methodD
+        a <- authD
+        b <- bodyD
+        liftIO (writeIORef cleanupRef $ cleanupD b)


I would feel better if we were able to use something like bracket for this. Not sure that's easily doable.

For writing the IORef, or for the whole sequence of operations?

Note that this is going to change a bit. @jkarni pointed out that adding clean up actions can happen from within bodyD and that we should not wait for it to terminate in order to add a clean up action, because if an exception occurs the cleanup never gets run. Particularly annoying in the case of file uploads where files can pile up in /tmp.

alpmestan · 2016-10-24T09:58:22Z

@soenkehahn Yes, I'll add tests that mimic my use case as far as Delayed/cleanup are concerned. In the meantime, you might want to take a look here: https://github.com/haskell-servant/servant-multipart/blob/master/src/Servant/Multipart.hs#L267

jkarni

I'm +1 on this approach, but of course we need tests!

jkarni · 2016-10-24T21:26:16Z

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

+newCleanupRef = CleanupRef <$> newIORef (return ())
+
+-- | Add a clean up action to a 'CleanupRef'
+addCleanup' :: IO () -> CleanupRef -> IO ()


I'd just have this is a where clause to addCleanup so it's obvious that it's not meant to be used elsewhere.

jkarni · 2016-10-24T21:30:38Z

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

-      Route app   -> app request (respond . Route)
-      Fail a      -> respond $ Fail a
-      FailFatal e -> respond $ FailFatal e
+    -- note: a Raw application doesn't register any cleanup


I think this is not just for consistency. E.g.:

type API = FileUpload :> Raw

Isn't in this case not having this going to cause the cleanup action to not be run? (It probably makes sense to have a test for (1) cleanup being run for Raw; (2) cleanup only being run once)

…n use the result of bodyD to perform some IO clean up operation

… allowing early clean up registration

alpmestan · 2017-01-12T01:56:34Z

Added some basic tests to make sure that clean up operations are run under various circumstances, among which the presence of exceptions in bodyD or serverD. Also rebased against master. Could anyone take a look and let me know what's missing?

phadej · 2017-01-16T13:47:43Z

servant-server/test/Servant/Server/Internal/RoutingApplicationSpec.hs

+  , methodD = DelayedIO $ \_req_ _cl -> ok
+  , authD = DelayedIO $ \_req _cl -> ok
+  , bodyD = do
+      liftIO (writeFile "delayed.test" "hia")


I'd prefer IORef, so the tests don't write to disk.

jkarni · 2017-01-18T19:13:24Z

@phadej @alpmestan is this superseded by more recent PRs? Can we close?

phadej · 2017-01-18T21:38:14Z

AFAICS all comments here are addressed already.

alpmestan mentioned this pull request Oct 21, 2016

Can't do IO while preparing a Req (servant-client) ? #624

Closed

jkarni requested changes Oct 21, 2016

View reviewed changes

soenkehahn reviewed Oct 23, 2016

View reviewed changes

jkarni requested changes Oct 24, 2016

View reviewed changes

alpmestan mentioned this pull request Jan 7, 2017

File upload combinator #133

Closed

alpmestan added 4 commits January 12, 2017 01:25

add a field in Delayed that lets us specify a clean up action that ca…

5e2f8be

…n use the result of bodyD to perform some IO clean up operation

fix a warning

4a52cce

make cleanup in Delayed more resistant to exceptions

9beedb5

use an ioref to store clean up actions instead of a field in Delayed,…

81a876c

… allowing early clean up registration

alpmestan force-pushed the delayed-cleanup branch from bd318c5 to 81a876c Compare January 12, 2017 00:25

add some basic tests for the cleanup machinery in Delayed

6aab9be

alpmestan force-pushed the delayed-cleanup branch from 12a1dc1 to 6aab9be Compare January 12, 2017 01:58

warning free

2769031

phadej modified the milestone: 0.10 Jan 13, 2017

phadej reviewed Jan 16, 2017

View reviewed changes

phadej mentioned this pull request Jan 17, 2017

Delayed cleanup ioref tests #674

Merged

phadej closed this Jan 18, 2017

jkarni deleted the delayed-cleanup branch February 1, 2018 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a field to Delayed for storing a clean up action #622

Add a field to Delayed for storing a clean up action #622

alpmestan commented Oct 21, 2016

jkarni left a comment

jkarni Oct 21, 2016

alpmestan Oct 21, 2016

jkarni Oct 21, 2016

alpmestan Oct 21, 2016

alpmestan Oct 21, 2016

jkarni Oct 21, 2016

alpmestan Oct 21, 2016 •

edited

Loading

alpmestan commented Oct 21, 2016

soenkehahn left a comment

soenkehahn Oct 23, 2016

soenkehahn Oct 23, 2016

alpmestan Oct 24, 2016

alpmestan Oct 24, 2016

alpmestan commented Oct 24, 2016

jkarni left a comment

jkarni Oct 24, 2016

phadej Jan 16, 2017

jkarni Oct 24, 2016

alpmestan commented Jan 12, 2017

phadej Jan 16, 2017

jkarni commented Jan 18, 2017

phadej commented Jan 18, 2017

Add a field to Delayed for storing a clean up action #622

Add a field to Delayed for storing a clean up action #622

Conversation

alpmestan commented Oct 21, 2016

jkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpmestan Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

alpmestan commented Oct 21, 2016

soenkehahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpmestan commented Oct 24, 2016

jkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpmestan commented Jan 12, 2017

Choose a reason for hiding this comment

jkarni commented Jan 18, 2017

phadej commented Jan 18, 2017

alpmestan Oct 21, 2016 •

edited

Loading