Skip to content
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

fix exception handling of downloads in new-build #3630

Merged
merged 5 commits into from
Jul 28, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion cabal-install/Distribution/Client/FetchUtils.hs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ module Distribution.Client.FetchUtils (
checkRepoTarballFetched,
fetchRepoTarball,

-- ** fetching packages asynchronously
asyncFetchPackages,
waitAsyncFetchPackage,
AsyncFetchMap,

-- * fetching other things
downloadIndex,
) where
Expand All @@ -35,7 +40,7 @@ import Distribution.Client.HttpUtils
import Distribution.Package
( PackageId, packageName, packageVersion )
import Distribution.Simple.Utils
( notice, info, setupMessage )
( notice, info, debug, setupMessage )
import Distribution.Text
( display )
import Distribution.Verbosity
Expand All @@ -44,6 +49,12 @@ import Distribution.Client.GlobalFlags
( RepoContext(..) )

import Data.Maybe
import Data.Map (Map)
import qualified Data.Map as Map
import Control.Monad
import Control.Exception
import Control.Concurrent.Async
import Control.Concurrent.MVar
import System.Directory
( doesFileExist, createDirectoryIfMissing, getTemporaryDirectory )
import System.IO
Expand Down Expand Up @@ -185,6 +196,61 @@ downloadIndex transport verbosity remoteRepo cacheDir = do
downloadURI transport verbosity uri path


-- ------------------------------------------------------------
-- * Async fetch wrapper utilities
-- ------------------------------------------------------------

type AsyncFetchMap = Map UnresolvedPkgLoc
(MVar (Either SomeException ResolvedPkgLoc))

-- | Fork off an async action to download the given packages (by location).
--
-- The downloads are initiated in order, so you can arrange for packages that
-- will likely be needed sooner to be earlier in the list.
--
-- The body action is passed a map from those packages (identified by their
-- location) to a completion var for that package. So the body action should
-- lookup the location and use 'asyncFetchPackage' to get the result.
--
asyncFetchPackages :: Verbosity
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're using the async package, why not just actually use the combinators on Async to implement this? Using withAsync and then ignoring the passed in Async just seems extremely strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I used to do was fork them all off using async and put the async handles in the map. The problem with that is we don't actually wan to do them all concurrently, and also we want to do them in a particular order, to get the ones we need back soonest. I don't see any way to implement this while still fitting the async API.

-> RepoContext
-> [UnresolvedPkgLoc]
-> (AsyncFetchMap -> IO a)
-> IO a
asyncFetchPackages verbosity repoCtxt pkglocs body = do
--TODO: [nice to have] use parallel downloads?

asyncDownloadVars <- sequence [ do v <- newEmptyMVar
return (pkgloc, v)
| pkgloc <- pkglocs ]

let fetchPackages :: IO ()
fetchPackages =
forM_ asyncDownloadVars $ \(pkgloc, var) -> do
result <- try $ fetchPackage verbosity repoCtxt pkgloc
putMVar var result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the crucial bit really, for fixing exception handling. Previously the exception was propagated locally in the async action and so the callers picking up the async fetch package results would wait forever (or rather get an MVar deadlock exception).


withAsync fetchPackages $ \_ ->
body (Map.fromList asyncDownloadVars)


-- | Expect to find a download in progress in the given 'AsyncFetchMap'
-- and wait on it to finish.
--
-- If the download failed with an exception then this will be thrown.
--
waitAsyncFetchPackage :: Verbosity
-> AsyncFetchMap
-> UnresolvedPkgLoc
-> IO ResolvedPkgLoc
waitAsyncFetchPackage verbosity downloadMap srcloc =
case Map.lookup srcloc downloadMap of
Just hnd -> do
debug verbosity $ "Waiting for download of " ++ show srcloc
either throwIO return =<< takeMVar hnd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus this bit to rethrow.

Nothing -> fail "waitAsyncFetchPackage: package not being downloaded"


-- ------------------------------------------------------------
-- * Path utilities
-- ------------------------------------------------------------
Expand Down
Loading