Better recovery from a few index/tar errors #7972
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mikolaj
approved these changes
Feb 12, 2022
| @@ -365,7 +365,8 @@ readRepoIndex :: Verbosity -> RepoContext -> Repo -> RepoIndexState | |||
| readRepoIndex verbosity repoCtxt repo idxState = | |||
| handleNotFound $ do | |||
| when (isRepoRemote repo) $ warnIfIndexIsOld =<< getIndexFileAge repo | |||
| updateRepoIndexCache verbosity (RepoIndex repoCtxt repo) | |||
| -- note that if this step fails due to a bad repocache, the the procedure can still succeed by reading from the existing cache, which is updated regardless. | |||
| updateRepoIndexCache verbosity (RepoIndex repoCtxt repo) `catch` (\(e :: SomeException) -> warn verbosity $ "unable to update the repo index cache -- " ++ displayException e) | |||
There was a problem hiding this comment.
Would it be possible to make this line a bit shorter? Even github forces me to scroll to see it. ;)
There was a problem hiding this comment.
+1, also let's consistently use show or displayException? (unless there's a particular reason for the distinction I'm missing)
robx
reviewed
Feb 21, 2022
| @@ -209,7 +211,7 @@ updateRepo verbosity _updateFlags repoCtxt (repo, indexState) = do | |||
| case updated of | |||
| Sec.NoUpdates -> do | |||
| now <- getCurrentTime | |||
| setModificationTime (indexBaseName repo <.> "tar") now | |||
| setModificationTime (indexBaseName repo <.> "tar") now `catch` (\(e :: SomeException) -> warn verbosity $ "Could not set modification time of index tarball -- " ++ show e) | |||
There was a problem hiding this comment.
This also catches asynchronous exceptions, such as e.g. UserInterrupt issued when Ctrl-C is hit; would be more robust to use safe-exception's catch or catch some more targeted set of exceptions, e.g. using catchIO as below.
| else ioError e | ||
| else do | ||
| putStrLn $ "Warning: could not read current index timestamp: " ++ show e | ||
| return Nothing |
There was a problem hiding this comment.
can we pass in verbosity and also use warn here?
| @@ -365,7 +365,8 @@ readRepoIndex :: Verbosity -> RepoContext -> Repo -> RepoIndexState | |||
| readRepoIndex verbosity repoCtxt repo idxState = | |||
| handleNotFound $ do | |||
| when (isRepoRemote repo) $ warnIfIndexIsOld =<< getIndexFileAge repo | |||
| updateRepoIndexCache verbosity (RepoIndex repoCtxt repo) | |||
| -- note that if this step fails due to a bad repocache, the the procedure can still succeed by reading from the existing cache, which is updated regardless. | |||
| updateRepoIndexCache verbosity (RepoIndex repoCtxt repo) `catch` (\(e :: SomeException) -> warn verbosity $ "unable to update the repo index cache -- " ++ displayException e) | |||
There was a problem hiding this comment.
+1, also let's consistently use show or displayException? (unless there's a particular reason for the distinction I'm missing)
|
Thanks for the comments. Addressed 'em all. |
Mikolaj
added
merge me
|
@Mergifyio rebase master |
✅ Branch has been successfully rebased |
jneira
force-pushed
the
gb/recover-some-tar-errors
branch
from
324f2d2 to
c584327
Compare
|
@Mergifyio rebase |
☑️ Nothing to do
|
|
CI jobs are stuck, so let me merge manually. |
Kleidukos
pushed a commit
to Kleidukos/cabal
that referenced
this pull request
Mar 30, 2022
* don't crash on a few stray exceptions * try -> catch and display * act on reviewer comments Co-authored-by: Gershom Bazerman <[email protected]>
andreabedini
pushed a commit
to andreabedini/cabal
that referenced
this pull request
May 5, 2022
* don't crash on a few stray exceptions * try -> catch and display * act on reviewer comments Co-authored-by: Gershom Bazerman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #5518 and #4987
Both these errors are in theory non-fatal, but simply gave uncaught exceptions. Cabal now warns on the exceptions and continues. This improves the resilience of cabal to bad cached index state from remote repositories, but does not solve the general problem completely. The most general path is to implement haskell/hackage-security#199