-
Notifications
You must be signed in to change notification settings - Fork 698
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
Changes from all commits
cdacc51
db78566
49b31ed
04eb173
c5db03c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,11 @@ module Distribution.Client.FetchUtils ( | |
checkRepoTarballFetched, | ||
fetchRepoTarball, | ||
|
||
-- ** fetching packages asynchronously | ||
asyncFetchPackages, | ||
waitAsyncFetchPackage, | ||
AsyncFetchMap, | ||
|
||
-- * fetching other things | ||
downloadIndex, | ||
) where | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
-> 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
-- ------------------------------------------------------------ | ||
|
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.
If you're using the async package, why not just actually use the combinators on
Async
to implement this? UsingwithAsync
and then ignoring the passed inAsync
just seems extremely strange.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 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.