Skip to content

Delayed cleanup ioref tests #674

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

Merged
merged 7 commits into from
Jan 18, 2017

Conversation

phadej
Copy link
Contributor

@phadej phadej commented Jan 17, 2017

Rebase of #622, with tests changed to use IORef

@phadej
Copy link
Contributor Author

phadej commented Jan 17, 2017

I'm ok with merging this, and then rewriting DelayedIO to use

newtype DelayedIO a = DelayedIO { runDelayedIO' :: ReaderT Request (ResourceT IO) (RouteResult a) }
-- or even
newtype DelayedIO a = DelayedIO { runDelayedIO' :: ReaderT Request (ResourceT (RouteResultT IO)) a }

opinions on this, @alpmestan, @jkarni?


for the reference: newtype ResourceT m a = ResourceT { unResourceT :: I.IORef ReleaseMap -> m a } from http://hackage.haskell.org/package/resourcet

@phadej phadej force-pushed the delayed-cleanup-ioref-tests branch from c31e783 to 85f5000 Compare January 17, 2017 21:13
@alpmestan
Copy link
Contributor

alpmestan commented Jan 18, 2017

There's some build failure to address (https://travis-ci.org/haskell-servant/servant/jobs/192826872#L393) -- for ghc 7.8. Other than that, LGTM. We can merge once it's fixed.

@jkarni
Copy link
Member

jkarni commented Jan 18, 2017

Why not just use ResourceT (and remove CleanupRef) from the start? It seems like we're just mimicking ResourceTs functionality, which, given how subtle these things can be (e.g., what happens in the presence of forks), is probably going to bite us.

@phadej phadej force-pushed the delayed-cleanup-ioref-tests branch 2 times, most recently from d2614d0 to 87707a3 Compare January 18, 2017 08:06
@phadej phadej force-pushed the delayed-cleanup-ioref-tests branch from 87707a3 to 60ee1ab Compare January 18, 2017 08:26
@phadej phadej merged commit 6d0aa92 into haskell-servant:master Jan 18, 2017
@phadej phadej deleted the delayed-cleanup-ioref-tests branch January 18, 2017 08:36
@phadej
Copy link
Contributor Author

phadej commented Jan 18, 2017

@jkarni I'll make a refactor PR today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants