diff --git a/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt b/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt index 495b52fb..616eb541 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt @@ -8,6 +8,7 @@ import okhttp3.Request import java.io.File import java.io.IOException import java.util.Base64 +import java.util.concurrent.TimeUnit /** * Cover art downloaded alongside the media, stored per source under `downloads//art`. @@ -68,8 +69,12 @@ fun interface ArtworkFetcher { } class OkHttpArtworkFetcher( - private val http: OkHttpClient, + http: OkHttpClient, ) : ArtworkFetcher { + // A blocking OkHttp call is not interrupted when the coroutine around it is cancelled, so the + // per-call timeout is what actually bounds a stuck artwork request. + private val http = http.newBuilder().callTimeout(CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS).build() + override suspend fun fetch(url: String): ByteArray = withContext(Dispatchers.IO) { val request = Request.Builder().url(url).build() @@ -78,4 +83,8 @@ class OkHttpArtworkFetcher( response.body?.bytes() ?: throw IOException("Artwork response had no body") } } + + private companion object { + const val CALL_TIMEOUT_SECONDS = 15L + } } diff --git a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt index c3f1fdcd..c69b5e9a 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -99,17 +99,24 @@ class DownloadRepository( showMessage(NO_ADDRESS) return@withContext } - // Artwork runs in our process and is fetched while the app is in the foreground; the - // media itself is handed to the platform, which keeps going without us. - runCatching { withTimeoutOrNull(ART_TIMEOUT_MS) { fetchArtwork(sourceId, songId) } } + if (!enqueueIfAbsent(sourceId, songId)) return@withContext + // The media is already with the platform, so this only has to happen while we are still + // in the foreground: artwork is fetched by us, and it must not hold up the download. + fetchArtworkSafely(sourceId, songId) + } + + private suspend fun enqueueIfAbsent( + sourceId: Long, + songId: String, + ): Boolean = mutex.withLock { - if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock + if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock false val existing = db.downloadDao().find(sourceId, songId) existing?.engineId?.let(engine::cancel) - val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock + val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock false enqueue(sourceId, songId, url) + true } - } suspend fun remove( sourceId: Long, @@ -139,14 +146,14 @@ class DownloadRepository( suspend fun reconcile() = withContext(dispatcher) { - mutex.withLock { - db.downloadDao().all().forEach { row -> - val state = row.engineId?.let(engine::download) - if (state == null) recover(row) else apply(row, state) + val resumed = + mutex.withLock { + val resumed = reconcileRows(db.downloadDao().all()) + sweep(db.downloadDao().all()) + ensurePolling() + resumed } - sweep(db.downloadDao().all()) - ensurePolling() - } + resumed.forEach { fetchArtworkSafely(it.sourceId, it.songId) } } private fun downloadStates(): Flow> = @@ -224,26 +231,48 @@ class DownloadRepository( ensurePolling() } - private suspend fun recover(row: SongDownload) { + private suspend fun recover(row: SongDownload): Boolean = when (row.status) { DownloadStatus.Completed -> { if (!file(row.sourceId, row.songId).exists()) markFailed(row, MISSING_FILE) + false } DownloadStatus.Queued -> { - if (sourceRepository.activeSourceIdOnce() != row.sourceId) return - val url = sourceRepository.downloadUri(row.sourceId, row.songId) ?: return + if (sourceRepository.activeSourceIdOnce() != row.sourceId) return false + val url = sourceRepository.downloadUri(row.sourceId, row.songId) ?: return false enqueue(row.sourceId, row.songId, url) + true } DownloadStatus.Running -> { markFailed(row, STOPPED) + false } DownloadStatus.Failed -> { - Unit + false } } + + private suspend fun reconcileRows(rows: List): List { + val resumed = ArrayList() + rows.forEach { row -> + val state = row.engineId?.let(engine::download) + if (state == null) { + if (recover(row)) resumed += row + } else { + apply(row, state) + } + } + return resumed + } + + private suspend fun fetchArtworkSafely( + sourceId: Long, + songId: String, + ) { + runCatching { withTimeoutOrNull(ART_TIMEOUT_MS) { fetchArtwork(sourceId, songId) } } } private suspend fun apply( @@ -283,15 +312,14 @@ class DownloadRepository( scope.launch { while (true) { delay(POLL_MS) - mutex.withLock { - val sourceId = sourceRepository.activeSourceIdOnce() - val active = db.downloadDao().all().filter { it.status.isActive && it.sourceId == sourceId } - if (active.isEmpty()) return@launch - active.forEach { row -> - val state = row.engineId?.let(engine::download) - if (state == null) recover(row) else apply(row, state) + val resumed = + mutex.withLock { + val sourceId = sourceRepository.activeSourceIdOnce() + val active = db.downloadDao().all().filter { it.status.isActive && it.sourceId == sourceId } + if (active.isEmpty()) return@launch + reconcileRows(active) } - } + resumed.forEach { fetchArtworkSafely(it.sourceId, it.songId) } } } } @@ -331,6 +359,6 @@ class DownloadRepository( const val NO_ADDRESS = "Can't download: the server address is unavailable" const val STOPPED = "The download stopped unexpectedly" const val NOT_ACTIVE = "Can't download from a source that isn't active" - const val ART_TIMEOUT_MS = 10_000L + const val ART_TIMEOUT_MS = 60_000L } } diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 5026c135..c00741e4 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -46,10 +46,12 @@ class DownloadRepositoryTest { private lateinit var dir: File private val messages = CopyOnWriteArrayList() private val requestedArt = CopyOnWriteArrayList() + private val artFetchedAfterTheEngineRequest = CopyOnWriteArrayList() private var failArtworkFetch = false private val fetcher = ArtworkFetcher { url -> requestedArt += url + artFetchedAfterTheEngineRequest += engine.requests.isNotEmpty() if (failArtworkFetch) throw IOException("boom") byteArrayOf(1, 2, 3) } @@ -312,6 +314,29 @@ class DownloadRepositoryTest { } } + @Test + fun theDownloadStartsBeforeItsArtworkIsFetched() = + runTest { + seedLibrary() + + runBlocking { repository.download(1, "s1") } + + assertTrue(artFetchedAfterTheEngineRequest.isNotEmpty()) + assertTrue(artFetchedAfterTheEngineRequest.all { it }) + } + + @Test + fun resumingAQueuedDownloadAlsoStoresItsArtwork() = + runTest { + seedLibrary() + db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Queued, engineId = 7)) + + runBlocking { repository.reconcile() } + + assertEquals(1, engine.requests.size) + assertNotNull(artwork.uri(1, coverArtKey(1, ALBUM_ART, false))) + } + @Test fun aSongWithoutArtworkStillDownloads() = runTest {