Skip to content

Commit

Permalink
Add finalizer for reaper thread
Browse files Browse the repository at this point in the history
  • Loading branch information
snoyberg committed Aug 4, 2013
1 parent 45da488 commit 5929373
Showing 1 changed file with 10 additions and 7 deletions.
17 changes: 10 additions & 7 deletions Network/HTTP/Conduit/Manager.hs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import Network.Socks5 (SocksConf)
import Data.Default
import Data.Maybe (mapMaybe)
import System.IO (Handle)
import System.Mem.Weak (addFinalizer)
import Data.Conduit (($$), yield, runException)

-- | Settings for a @Manager@. Please use the 'def' function and then modify
Expand Down Expand Up @@ -185,13 +186,15 @@ newManager ms = do
mapRef <- I.newIORef (Just Map.empty)
certCache <- I.newIORef Map.empty
_ <- forkIO $ reap mapRef certCache
return Manager
{ mConns = mapRef
, mMaxConns = managerConnCount ms
, mCheckCerts = \x y -> getCertStore >>= \cs -> managerCheckCerts ms cs x y
, mCertCache = certCache
, mResponseTimeout = managerResponseTimeout ms
}
let manager = Manager
{ mConns = mapRef
, mMaxConns = managerConnCount ms
, mCheckCerts = \x y -> getCertStore >>= \cs -> managerCheckCerts ms cs x y
, mCertCache = certCache
, mResponseTimeout = managerResponseTimeout ms
}
addFinalizer manager $ closeManager manager
return manager

-- | Collect and destroy any stale connections.
reap :: I.IORef (Maybe (Map.Map ConnKey (NonEmptyList ConnInfo)))
Expand Down

4 comments on commit 5929373

@nezetic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addFinalizer make the Manager not working anymore in my projects (Mac Os, ghc 7.4.1).

Debug.Trace show that closeManager is called (by garbage collector ?) while computations are still running in ResourceT, and consequently a new socket is created for each connection established (to the same server) inside withManager function.

@snoyberg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you by any chance have a reproducing test case for this?

@snoyberg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and this should definitely be opened as an issue, it's a serious bug. Thanks for bringing it to my attention.

@nezetic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your fast reply :)

I've opened an issue (#140)

Please sign in to comment.