-
Notifications
You must be signed in to change notification settings - Fork 191
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
http-conduit: Store the cleanup actions of responses in ResourceT
#539
http-conduit: Store the cleanup actions of responses in ResourceT
#539
Conversation
We're testing this for now, but will mark it as ready once we're sure it works. Opinions and comments welcome. |
Because `ResourceT` needs to hold onto the `Response` so it can perform cleanup when leaving `runResourceT`, move the cleanup action from the `Response` itself and register it with `ResourceT`. Then all code paths which trigger cleanup (leaving `ResourceT`, consuming the body, calling `responseClose`) will do so by releasing the `Response` from the `ReleaseMap`, preventing a space leak.
d7e429d
to
dc2051a
Compare
Here is the test program. {-# LANGUAGE OverloadedLabels #-}
{-# LANGUAGE OverloadedStrings #-}
module Main where
import Amazonka qualified
import Amazonka.S3 qualified as S3
import Control.Monad (void)
import Control.Monad.Random.Class
import Control.Monad.Trans.Class (lift)
import Data.Generics.Labels ()
import Data.Text qualified as T
import Data.Traversable (for)
main :: IO ()
main = do
env <- Amazonka.newEnv Amazonka.discover
void . Amazonka.runResourceT $
for (replicate 1000 ()) $ \_ -> do
randomStr <- fmap (T.pack . take 10000000) . lift $ getRandomRs ('a', 'z')
lift $ putStrLn "Sending 10MB of data"
void . Amazonka.send env $
S3.newPutObject "bellroy-eventlog-test" "test" (Amazonka.toBody randomStr) Test command: |
This definitely improves things, so I'm un-marking it as draft. |
I'm not following the change here. The existing |
In This PR changes that by replacing the We can't use
|
Ah, got it, thanks for the explanation. Makes sense, and this looks good! Merging. |
Because
ResourceT
needs to hold onto theResponse
so it can perform cleanup when leavingrunResourceT
, move the cleanup action from theResponse
itself and register it withResourceT
. Then all code paths which trigger cleanup (leavingResourceT
, consuming the body, callingresponseClose
) will do so by releasing theResponse
from theReleaseMap
, preventing a space leak.Fixes #537