-
Notifications
You must be signed in to change notification settings - Fork 304
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
Retry on ChecksumError #797
Changes from 16 commits
31bb91f
0f3b14c
8481769
db7341c
51e5fb4
67ffe13
0369f70
80c59d7
511e153
89a8be4
434b19b
379edb2
425319d
0bc4640
d631e9e
2aa22f9
ef97d88
3b61c58
85b5688
b239d92
36c21e0
f8bc798
8e8f5ac
5d9fb1a
de84b49
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ jar_library( | |
rev = "2.12.4", | ||
), | ||
], | ||
scope = 'runtime' | ||
) | ||
|
||
jar_library( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ object Cache { | |
} | ||
|
||
// java.nio.charset.StandardCharsets.UTF_8 not available in Java 6 | ||
private val UTF_8 = Charset.forName("UTF-8") | ||
val UTF_8: Charset = Charset.forName("UTF-8") | ||
|
||
// Check SHA-1 if available, else be fine with no checksum | ||
val defaultChecksums = Seq(Some("SHA-1"), None) | ||
|
@@ -518,7 +518,7 @@ object Cache { | |
doTouchCheckFile(file) | ||
Right(false) | ||
} | ||
case other => | ||
case other =>FileError | ||
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. ? 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. Reverted. This is weird...CI passed on that. |
||
Task.now(other) | ||
} | ||
} | ||
|
@@ -939,21 +939,28 @@ object Cache { | |
} | ||
} | ||
|
||
/** | ||
* This method computes the task needed to get a file. | ||
* | ||
* Retry only applies to [[coursier.FileError.WrongChecksum]]. | ||
* | ||
* [[coursier.FileError.DownloadError]] is handled separately at [[downloading]] | ||
*/ | ||
def file( | ||
artifact: Artifact, | ||
cache: File = default, | ||
cachePolicy: CachePolicy = CachePolicy.UpdateChanging, | ||
checksums: Seq[Option[String]] = defaultChecksums, | ||
logger: Option[Logger] = None, | ||
pool: ExecutorService = defaultPool, | ||
ttl: Option[Duration] = defaultTtl | ||
ttl: Option[Duration] = defaultTtl, | ||
retry: Int = 1 | ||
): EitherT[Task, FileError, File] = { | ||
|
||
implicit val pool0 = pool | ||
val checksums0: Seq[Option[String]] = if (checksums.isEmpty) Seq(None) else checksums | ||
|
||
val checksums0 = if (checksums.isEmpty) Seq(None) else checksums | ||
|
||
val res = EitherT { | ||
val res: EitherT[Task, FileError, (File, Option[String])] = EitherT { | ||
download( | ||
artifact, | ||
cache, | ||
|
@@ -987,10 +994,35 @@ object Cache { | |
} | ||
|
||
res.flatMap { | ||
case (f, None) => EitherT(Task.now[Either[FileError, File]](Right(f))) | ||
case (f, Some(c)) => | ||
validateChecksum(artifact, c, cache, pool).map(_ => f) | ||
case (file0, None) => EitherT(Task.now[Either[FileError, File]](Right(file0))) | ||
case (file0, Some(shaType)) => | ||
validateChecksum(artifact, shaType, cache, pool).map(_ => file0) | ||
}.leftFlatMap { | ||
case err: FileError.WrongChecksum => | ||
if (retry == 0) { | ||
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.
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. Done |
||
logger.foreach(_.log(s"Retry exhausted for ${artifact.url}\n", None)) | ||
EitherT(Task.now[Either[FileError, File]](Left(err))) | ||
} | ||
else { | ||
val badFile = localFile(artifact.url, cache, artifact.authentication.map(_.user)) | ||
badFile.delete() | ||
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 call should be wrapped in a S.schedule(pool) {
badFile.delete()
logger.foreach(…)
}.flatMap { _ =>
file(…)
} 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. done |
||
logger.foreach(_.log(s"Bad file deleted: ${badFile.getAbsolutePath} due to wrong checksum. Retrying with count $retry...\n", None)) | ||
file( | ||
artifact, | ||
cache, | ||
cachePolicy, | ||
checksums, | ||
logger, | ||
pool, | ||
ttl, | ||
retry - 1 | ||
) | ||
} | ||
case err => | ||
EitherT(Task.now[Either[FileError, File]](Left(err))) | ||
} | ||
|
||
|
||
} | ||
|
||
def fetch( | ||
|
@@ -1141,6 +1173,7 @@ object Cache { | |
def downloadedArtifact(url: String, success: Boolean): Unit = {} | ||
def checkingUpdates(url: String, currentTimeOpt: Option[Long]): Unit = {} | ||
def checkingUpdatesResult(url: String, currentTimeOpt: Option[Long], remoteTimeOpt: Option[Long]): Unit = {} | ||
def log(content: String, currentTimeOpt: Option[Long]): Unit = {} | ||
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. I think it'd be better to have a more specific method for the error we're reporting… like def removedCorruptFile(url: String, file: File, reason: Option[FileError]): Unit |
||
} | ||
|
||
object Logger { | ||
|
@@ -1180,6 +1213,8 @@ object Cache { | |
logger.checkingUpdates(url, currentTimeOpt) | ||
override def checkingUpdatesResult(url: String, currentTimeOpt: Option[Long], remoteTimeOpt: Option[Long]) = | ||
logger.checkingUpdatesResult(url, currentTimeOpt, remoteTimeOpt) | ||
override def log(content: String, currentTimeOpt: Option[Long]): Unit = | ||
logger.log(content, currentTimeOpt) | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -509,4 +509,9 @@ class TermDisplay( | |
} | ||
} | ||
|
||
override def log(content: String, currentTimeOpt: Option[Long]): Unit = { | ||
updateRunnable.newEntry(content, CheckUpdateInfo(currentTimeOpt, None, isDone = true), content) | ||
updateRunnable.removeEntry(content, success = true, fallbackMessage = content)(x => x) | ||
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. Not entirely sure if this is the right way to log things. Please kindly advise. 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. Let's keep an empty implem for now (with a more specific method), we'll see how we can report that (with progress bars and all) later… |
||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ import coursier.Cache | |
|
||
final case class CacheOptions( | ||
@Help("Cache directory (defaults to environment variable COURSIER_CACHE or ~/.coursier/cache/v1)") | ||
@Short("C") | ||
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. Removing this because conflicting with 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. I agree. A helper method could be added for that in case-app. |
||
cache: String = Cache.default.toString | ||
) | ||
|
||
|
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.
Just FYI, with Java >= 7,
java.nio.charset.StandardCharsets.UTF_8
is available for that. I think I'll push a commit using the latter everywhere soon (while dropping support for Java 6 & 7). You can use it right now if you'd like.