fix exception handling of downloads in new-build #3630
Conversation
As a result of the previous InstallPlan refactoring, we can now use the non-serialisable BuildFailure type from D.C.Types which uses SomeException, where previously we had to use a copy of that type that used String for the errors. So now there's no longer any need to have a separate set of types for BuildResult, BuildResults, BuildSuccess or BuildFailure. There was a minor difference in the structure of the BuildSuccess, where in the new build code we need to be able to produce the InstalledPackageInfo at a different point from the rest of the info in the BuildSuccess. This can be kept local to the ProjecBuilding module, but accounts for the somewhat larger number of changes in that module.
| Just hnd -> do | ||
| debug verbosity $ "Waiting for download of " ++ show srcloc | ||
| either throwIO return =<< takeMVar hnd | ||
| Nothing -> fail "waitAsyncFetchPackage: package not being download" |
Split things up a little so the generic async fetch can live with the other fetch utils. This also makes it easier to test. Change the exception handling so that any exception in fetching is propagated when collecting the fetch result.
Download errors are now put into the residual install plan, like other build errors. Fixes issue haskell#3387
dcoutts
force-pushed
the
new-build-exception-handling
branch
from
0fedb42 to
49b31ed
Compare
| -- 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I don't really understand the patch, but if it fixes the exceptions problem 👍 |
Which one, the first one or the downloads one? Patch 2 does two things: it moves code around and tidies things up, and also it makes sure to catch exceptions from |
| fetchPackages = | ||
| forM_ asyncDownloadVars $ \(pkgloc, var) -> do | ||
| result <- try $ fetchPackage verbosity repoCtxt pkgloc | ||
| putMVar var result |
There was a problem hiding this comment.
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).
|
My lack of understanding is mostly cdacc51; there's some shuffling around of types, but I don't really know how things were structured previously, or how they are structured now. But... code got removed, so I'm happy to take your word for it ;) |
|
So the new-build code previously used: whereas the common D.C.Types used by the D.C.Install etc code uses: So the main difference (apart from using Either vs a custom 2-constructor type) is that the previous new-build code put the |
|
I don't understand the travis failure on 7.4.2. It looks like it's cabal sdist that is failing, but the code in this PR is completely unrelated to sdist. |
|
I think it's OK if we disable sdist test in the main test; I've found that it doesn't work very reliably if there is configuration (as there is here) for a differing version of Cabal; if you clean it works. We do have a test for sdist in bootstrap. |
|
Ok to merge then? |
|
Yep, but you or I will have to immediately push a commit to remove the sdist from |
|
LGTM as well. |
Partly this is a continuation of the InstallPlan refactoring #3622, but also a related change to fix #3387, by handling exceptions during download properly.
These things are also a step towards finishing #3394 properly, since it means we now have all the
BuildResultsusing proper exception types and so will be able to use that to report at the end what if anything when wrong.