-
-
Notifications
You must be signed in to change notification settings - Fork 409
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
Use resourcet for resource managment #675
Conversation
r <- runDelayed rawApplication env request cleanupRef | ||
go r request respond `finally` runCleanup cleanupRef | ||
r <- runDelayed rawApplication env request | ||
go r request respond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, here I guess I'll run cleanups too early. Wouldn't it?
@@ -264,18 +303,17 @@ passToServer Delayed{..} x = | |||
runDelayed :: Delayed env a | |||
-> env | |||
-> Request | |||
-> CleanupRef | |||
-> IO (RouteResult a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should this return ResourceT IO (RouteResult a)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments, but overall this looks great!
@@ -65,7 +63,7 @@ steps: | |||
|
|||
# A common setting is the number of columns (parts of) code will be wrapped | |||
# to. Different steps take this into account. Default: 80. | |||
columns: 80 | |||
columns: 120 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to change this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, except with current style the import lists are almost always spanning multiple lines, which I find annoying. There's a bit of settings in newest stylish-haskell
so if the line is too long, imports could be aligned as
import Data.List (foldl')
import qualified VeryLongNamespace.Data.Foo.Bar
(foo, bar, quux, longMethodName, anotherLongMethodName, Foobar,
MonadEveryhing, zyxxy)
or at least I remember making some such configuration possible.
I'll revert this change and investigate more (subjectively) pleasant (yet diff friendly) formatting style.
instance MonadThrow m => MonadThrow (RouteResultT m) where | ||
throwM = lift . throwM | ||
|
||
-- instance MonadCatch m => MonadCatch (RouteResultT m) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these.
newtype RouteResultT m a = RouteResultT { runRouteResultT :: m (RouteResult a) } | ||
deriving (Functor) | ||
|
||
-- As we write these instances, we get instances for `DelayedIO` with GND. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the comment is necessary. Instances are always good in my opinion.
pure = return | ||
(<*>) = ap | ||
returnRouteResult :: RouteResult a -> DelayedIO a | ||
returnRouteResult x = DelayedIO $ lift . lift $ RouteResultT . return $ x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the name, but can't think of anything better (maybe delayRouteResult
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's about liftRouteResult
?
it "writes and cleanups resources" $ do | ||
request "GET" "foobar" [] "" `shouldRespondWith` "foobar" | ||
liftIO $ do | ||
cleanUpDone <- isJust <$> readIORef delayedTestRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can in theory pass if check
in line 67 were never run. Can we maybe write something other than Nothing
(the initial ref state) to be sure the value is coming from deallocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a IORef
three way, None | InUse a | Cleared
.
|
||
------------------------------------------------------------------------------- | ||
-- Spec | ||
------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely don't insist on it, but it perhaps would be nice to have a test where the combinator allocating the resource forks a thread, and that thread waits on an MVar (with readMVar
). Then the test checks (a) that before it putMVar
s, the resource is not yet deallocated, and (b) after it putMVar
s, the resource is deallocated.
What I have in mind is forking jobs that are longer running, which I imagine would be commonly used in conjunction with resources, especially if we make Handler
a MonadResource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not understand, could you write an example, assuming that Handler
is MonadResource
?
runDelayedIO | ||
(do c <- capturesD env | ||
methodD | ||
a <- authD | ||
b <- bodyD | ||
DelayedIO $ \ r _cleanup -> return (serverD c a b r) | ||
r <- ask | ||
returnRouteResult (serverD c a b r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that Handler
is abstract, and that this is being added, I feel like it makes sense to make Handler
a MonadResource
so that handlers can also register, allocate, and release. That said, that can just as well be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handler
cannot, it's just ExceptT ServerErr IO a
. Then we'll need to add ResourceT
there too. I'm neutral about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(There's no technical problem adding it, the runHandler
is inside runResourceT
anyway).
No description provided.