Fix downloads cancellation #210

app/src/main/java/org/koitharu/kotatsu/download/domain/DownloadManager.kt

@@ -9,8 +9,10 @@ import java.io.File
 import kotlinx.coroutines.*
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.sync.Semaphore
+import kotlinx.coroutines.sync.withPermit
 import okhttp3.OkHttpClient
 import okhttp3.Request
+import okhttp3.internal.closeQuietly
 import okio.IOException
 import org.koitharu.kotatsu.R
 import org.koitharu.kotatsu.core.network.CommonHeaders
@@ -59,25 +61,34 @@ class DownloadManager(
 			DownloadState.Queued(startId = startId, manga = manga, cover = null),
 		)
 		val pausingHandle = PausingHandle()
-		val job = downloadMangaImpl(manga, chaptersIds?.takeUnless { it.isEmpty() }, stateFlow, pausingHandle, startId)
+		val job = coroutineScope.launch(Dispatchers.Default + errorStateHandler(stateFlow)) {
+			try {
+				downloadMangaImpl(manga, chaptersIds?.takeUnless { it.isEmpty() }, stateFlow, pausingHandle, startId)
+			} catch (e: CancellationException) { // handle cancellation if not handled already
+				val state = stateFlow.value
+				if (state !is DownloadState.Cancelled) {
+					stateFlow.value = DownloadState.Cancelled(startId, state.manga, state.cover)
+				}
+				throw e
+			}
+		}
 		return PausingProgressJob(job, stateFlow, pausingHandle)
 	}
 
-	private fun downloadMangaImpl(
+	private suspend fun downloadMangaImpl(
 		manga: Manga,
 		chaptersIds: LongArray?,
 		outState: MutableStateFlow<DownloadState>,
 		pausingHandle: PausingHandle,
 		startId: Int,
-	): Job = coroutineScope.launch(Dispatchers.Default + errorStateHandler(outState)) {
+	) {
 		@Suppress("NAME_SHADOWING")
 		var manga = manga
 		val chaptersIdsSet = chaptersIds?.toMutableSet()
 		val cover = loadCover(manga)
 		outState.value = DownloadState.Queued(startId, manga, cover)
-		localMangaRepository.lockManga(manga.id)
+		withMangaLock(manga) {
-		semaphore.acquire()
+			semaphore.withPermit {
-		coroutineContext[WakeLockNode]?.acquire()
 				outState.value = DownloadState.Preparing(startId, manga, null)
 				val destination = localMangaRepository.getOutputDir()
 				checkNotNull(destination) { context.getString(R.string.cannot_find_available_storage) }
@@ -85,7 +96,8 @@ class DownloadManager(
 				var output: CbzMangaOutput? = null
 				try {
 					if (manga.source == MangaSource.LOCAL) {
-				manga = localMangaRepository.getRemoteManga(manga) ?: error("Cannot obtain remote manga instance")
+						manga = localMangaRepository.getRemoteManga(manga)
+							?: error("Cannot obtain remote manga instance")
 					}
 					val repo = MangaRepository(manga.source)
 					outState.value = DownloadState.Preparing(startId, manga, cover)
@@ -149,11 +161,11 @@ class DownloadManager(
 					outState.value = DownloadState.Error(startId, manga, cover, e, false)
 				} finally {
 					withContext(NonCancellable) {
+						output?.closeQuietly()
 						output?.cleanup()
 						File(destination, tempFileName).deleteAwait()
-				coroutineContext[WakeLockNode]?.release()
+					}
-				semaphore.release()
+				}
-				localMangaRepository.unlockManga(manga.id)
 			}
 		}
 	}
@@ -203,6 +215,7 @@ class DownloadManager(
 
 	private fun errorStateHandler(outState: MutableStateFlow<DownloadState>) =
 		CoroutineExceptionHandler { _, throwable ->
+			throwable.printStackTraceDebug()
 			val prevValue = outState.value
 			outState.value = DownloadState.Error(
 				startId = prevValue.startId,
@@ -224,6 +237,13 @@ class DownloadManager(
 		).drawable
 	}.getOrNull()
 
+	private suspend inline fun <T> withMangaLock(manga: Manga, block: () -> T) = try {
+		localMangaRepository.lockManga(manga.id)
+		block()
+	} finally {
+		localMangaRepository.unlockManga(manga.id)
+	}
+
 	class Factory(
 		private val context: Context,
 		private val imageLoader: ImageLoader,

app/src/main/java/org/koitharu/kotatsu/download/domain/WakeLockNode.kt

@@ -1,25 +0,0 @@
-package org.koitharu.kotatsu.download.domain
-
-import android.os.PowerManager
-import kotlin.coroutines.AbstractCoroutineContextElement
-import kotlin.coroutines.CoroutineContext
-
-class WakeLockNode(
-	private val wakeLock: PowerManager.WakeLock,
-	private val timeout: Long,
-) : AbstractCoroutineContextElement(Key) {
-
-	init {
-		wakeLock.setReferenceCounted(true)
-	}
-
-	fun acquire() {
-		wakeLock.acquire(timeout)
-	}
-
-	fun release() {
-		wakeLock.release()
-	}
-
-	companion object Key : CoroutineContext.Key<WakeLockNode>
-}

app/src/main/java/org/koitharu/kotatsu/download/ui/DownloadsActivity.kt

@@ -1,21 +1,23 @@
 package org.koitharu.kotatsu.download.ui
 
+import android.content.ComponentName
 import android.content.Context
 import android.content.Intent
+import android.content.ServiceConnection
 import android.os.Bundle
+import android.os.IBinder
 import androidx.core.graphics.Insets
 import androidx.core.view.isVisible
 import androidx.core.view.updatePadding
+import androidx.lifecycle.DefaultLifecycleObserver
+import androidx.lifecycle.LifecycleOwner
 import androidx.lifecycle.lifecycleScope
-import kotlinx.coroutines.flow.flatMapLatest
+import kotlinx.coroutines.Job
-import kotlinx.coroutines.flow.flowOf
+import kotlinx.coroutines.launch
-import kotlinx.coroutines.flow.launchIn
-import kotlinx.coroutines.flow.onEach
 import org.koin.android.ext.android.get
 import org.koitharu.kotatsu.base.ui.BaseActivity
 import org.koitharu.kotatsu.databinding.ActivityDownloadsBinding
 import org.koitharu.kotatsu.download.ui.service.DownloadService
-import org.koitharu.kotatsu.utils.bindServiceWithLifecycle
 
 class DownloadsActivity : BaseActivity<ActivityDownloadsBinding>() {
 
@@ -26,30 +28,63 @@ class DownloadsActivity : BaseActivity<ActivityDownloadsBinding>() {
 		val adapter = DownloadsAdapter(lifecycleScope, get())
 		binding.recyclerView.setHasFixedSize(true)
 		binding.recyclerView.adapter = adapter
-		bindServiceWithLifecycle(
+		val connection = DownloadServiceConnection(adapter)
-			owner = this,
+		bindService(Intent(this, DownloadService::class.java), connection, 0)
-			service = Intent(this, DownloadService::class.java),
+		lifecycle.addObserver(connection)
-			flags = 0,
-		).service.flatMapLatest { binder ->
-			(binder as? DownloadService.DownloadBinder)?.downloads ?: flowOf(null)
-		}.onEach {
-			adapter.items = it?.toList().orEmpty()
-			binding.textViewHolder.isVisible = it.isNullOrEmpty()
-		}.launchIn(lifecycleScope)
 	}
 
 	override fun onWindowInsetsChanged(insets: Insets) {
 		binding.recyclerView.updatePadding(
 			left = insets.left,
 			right = insets.right,
-			bottom = insets.bottom
+			bottom = insets.bottom,
 		)
 		binding.toolbar.updatePadding(
 			left = insets.left,
-			right = insets.right
+			right = insets.right,
 		)
 	}
 
+	private inner class DownloadServiceConnection(
+		private val adapter: DownloadsAdapter,
+	) : ServiceConnection, DefaultLifecycleObserver {
+
+		private var collectJob: Job? = null
+
+		override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
+			collectJob?.cancel()
+			val binder = (service as? DownloadService.DownloadBinder)
+			collectJob = if (binder == null) {
+				null
+			} else {
+				lifecycleScope.launch {
+					binder.downloads.collect {
+						setItems(it)
+					}
+				}
+			}
+		}
+
+		override fun onServiceDisconnected(name: ComponentName?) {
+			collectJob?.cancel()
+			collectJob = null
+			setItems(null)
+		}
+
+		override fun onDestroy(owner: LifecycleOwner) {
+			super.onDestroy(owner)
+			collectJob?.cancel()
+			collectJob = null
+			owner.lifecycle.removeObserver(this)
+			unbindService(this)
+		}
+
+		private fun setItems(items: Collection<DownloadItem>?) {
+			adapter.items = items?.toList().orEmpty()
+			binding.textViewHolder.isVisible = items.isNullOrEmpty()
+		}
+	}
+
 	companion object {
 
 		fun newIntent(context: Context) = Intent(context, DownloadsActivity::class.java)

app/src/main/java/org/koitharu/kotatsu/download/ui/service/DownloadNotification.kt

@@ -59,6 +59,7 @@ class DownloadNotification(private val context: Context) {
 		val style = NotificationCompat.InboxStyle(groupBuilder)
 		var progress = 0f
 		var isAllDone = true
+		var isInProgress = false
 		groupBuilder.setVisibility(NotificationCompat.VISIBILITY_PUBLIC)
 		states.forEach { _, state ->
 			if (state.manga.isNsfw) {
@@ -79,20 +80,24 @@ class DownloadNotification(private val context: Context) {
 				}
 				is DownloadState.PostProcessing -> {
 					progress++
+					isInProgress = true
 					isAllDone = false
 					context.getString(R.string.processing_)
 				}
 				is DownloadState.Preparing -> {
 					isAllDone = false
+					isInProgress = true
 					context.getString(R.string.preparing_)
 				}
 				is DownloadState.Progress -> {
 					isAllDone = false
+					isInProgress = true
 					progress += state.percent
 					context.getString(R.string.percent_string_pattern, (state.percent * 100).format())
 				}
 				is DownloadState.Queued -> {
 					isAllDone = false
+					isInProgress = true
 					context.getString(R.string.queued)
 				}
 			}
@@ -104,13 +109,20 @@ class DownloadNotification(private val context: Context) {
 				).parseAsHtml(HtmlCompat.FROM_HTML_MODE_LEGACY),
 			)
 		}
-		progress /= states.size.toFloat()
+		progress = if (isInProgress) {
-		style.setBigContentTitle(context.getString(R.string.downloading_manga))
+			progress / states.size.toFloat()
+		} else {
+			1f
+		}
+		style.setBigContentTitle(
+			context.getString(if (isAllDone) R.string.download_complete else R.string.downloading_manga),
+		)
 		groupBuilder.setContentText(context.resources.getQuantityString(R.plurals.items, states.size, states.size()))
 		groupBuilder.setNumber(states.size)
 		groupBuilder.setSmallIcon(
-			if (isAllDone) android.R.drawable.stat_sys_download_done else android.R.drawable.stat_sys_download,
+			if (isInProgress) android.R.drawable.stat_sys_download else android.R.drawable.stat_sys_download_done,
 		)
+		groupBuilder.setAutoCancel(isAllDone)
 		when (progress) {
 			1f -> groupBuilder.setProgress(0, 0, false)
 			0f -> groupBuilder.setProgress(1, 0, true)
@@ -120,11 +132,11 @@ class DownloadNotification(private val context: Context) {
 	}
 
 	fun detach() {
-		manager.cancel(ID_GROUP)
+		if (states.isNotEmpty()) {
-		if (states.isNotEmpty() && Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
 			val notification = buildGroupNotification()
 			manager.notify(ID_GROUP_DETACHED, notification)
 		}
+		manager.cancel(ID_GROUP)
 	}
 
 	fun newItem(startId: Int) = Item(startId)

app/src/main/java/org/koitharu/kotatsu/download/ui/service/DownloadService.kt

@@ -18,7 +18,6 @@ import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import java.util.concurrent.TimeUnit
 import kotlinx.coroutines.flow.*
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.plus
 import org.koin.android.ext.android.get
 import org.koin.core.context.GlobalContext
 import org.koitharu.kotatsu.BuildConfig
@@ -29,7 +28,6 @@ import org.koitharu.kotatsu.core.model.parcelable.ParcelableManga
 import org.koitharu.kotatsu.core.prefs.AppSettings
 import org.koitharu.kotatsu.download.domain.DownloadManager
 import org.koitharu.kotatsu.download.domain.DownloadState
-import org.koitharu.kotatsu.download.domain.WakeLockNode
 import org.koitharu.kotatsu.parsers.model.Manga
 import org.koitharu.kotatsu.utils.ext.connectivityManager
 import org.koitharu.kotatsu.utils.ext.throttle
@@ -41,6 +39,7 @@ class DownloadService : BaseService() {
 
 	private lateinit var downloadManager: DownloadManager
 	private lateinit var downloadNotification: DownloadNotification
+	private lateinit var wakeLock: PowerManager.WakeLock
 
 	private val jobs = LinkedHashMap<Int, PausingProgressJob<DownloadState>>()
 	private val jobCount = MutableStateFlow(0)
@@ -50,11 +49,10 @@ class DownloadService : BaseService() {
 		super.onCreate()
 		isRunning = true
 		downloadNotification = DownloadNotification(this)
-		val wakeLock = (applicationContext.getSystemService(Context.POWER_SERVICE) as PowerManager)
+		wakeLock = (applicationContext.getSystemService(Context.POWER_SERVICE) as PowerManager)
 			.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "kotatsu:downloading")
-		downloadManager = get<DownloadManager.Factory>().create(
+		wakeLock.acquire(TimeUnit.HOURS.toMillis(8))
-			coroutineScope = lifecycleScope + WakeLockNode(wakeLock, TimeUnit.HOURS.toMillis(1)),
+		downloadManager = get<DownloadManager.Factory>().create(lifecycleScope)
-		)
 		DownloadNotification.createChannel(this)
 		startForeground(DownloadNotification.ID_GROUP, downloadNotification.buildGroupNotification())
 		val intentFilter = IntentFilter()
@@ -84,6 +82,7 @@ class DownloadService : BaseService() {
 
 	override fun onDestroy() {
 		unregisterReceiver(controlReceiver)
+		wakeLock.release()
 		isRunning = false
 		super.onDestroy()
 	}
@@ -129,8 +128,9 @@ class DownloadService : BaseService() {
 				}
 				if (job.isCancelled) {
 					notificationItem.dismiss()
-					jobs.remove(startId)
+					if (jobs.remove(startId) != null) {
 						jobCount.value = jobs.size
+					}
 				} else {
 					notificationItem.notify(job.progressValue, -1L)
 				}
@@ -164,8 +164,9 @@ class DownloadService : BaseService() {
 			when (intent?.action) {
 				ACTION_DOWNLOAD_CANCEL -> {
 					val cancelId = intent.getIntExtra(EXTRA_CANCEL_ID, 0)
-					jobs.remove(cancelId)?.cancel()
+					jobs[cancelId]?.cancel()
-					jobCount.value = jobs.size
+					// jobs.remove(cancelId)?.cancel()
+					// jobCount.value = jobs.size
 				}
 				ACTION_DOWNLOAD_RESUME -> {
 					val cancelId = intent.getIntExtra(EXTRA_CANCEL_ID, 0)
@@ -177,12 +178,12 @@ class DownloadService : BaseService() {
 
 	class DownloadBinder(service: DownloadService) : Binder(), DefaultLifecycleObserver {
 
-		private var downloadsStateFlow = MutableStateFlow<Collection<PausingProgressJob<DownloadState>>>(emptyList())
+		private var downloadsStateFlow = MutableStateFlow<List<PausingProgressJob<DownloadState>>>(emptyList())
 
 		init {
 			service.lifecycle.addObserver(this)
 			service.jobCount.onEach {
-				downloadsStateFlow.value = service.jobs.values
+				downloadsStateFlow.value = service.jobs.values.toList()
 			}.launchIn(service.lifecycleScope)
 		}
 

app/src/main/java/org/koitharu/kotatsu/local/domain/LocalMangaRepository.kt

@@ -86,7 +86,7 @@ class LocalMangaRepository(private val storageManager: LocalStorageManager) : Ma
 				entries.filter { x ->
 					!x.isDirectory && x.name.substringBeforeLast(
 						File.separatorChar,
-						""
+						"",
 					) == parent
 				}
 			}
@@ -138,11 +138,11 @@ class LocalMangaRepository(private val storageManager: LocalStorageManager) : Ma
 				url = fileUri,
 				coverUrl = zipUri(
 					file,
-					entryName = index.getCoverEntry() ?: findFirstImageEntry(zip.entries())?.name.orEmpty()
+					entryName = index.getCoverEntry() ?: findFirstImageEntry(zip.entries())?.name.orEmpty(),
 				),
 				chapters = info.chapters?.map { c ->
 					c.copy(url = fileUri, source = MangaSource.LOCAL)
-				}
+				},
 			)
 		}
 		// fallback
@@ -211,7 +211,7 @@ class LocalMangaRepository(private val storageManager: LocalStorageManager) : Ma
 					return@runInterruptible info.copy2(
 						source = MangaSource.LOCAL,
 						url = fileUri,
-						chapters = info.chapters?.map { c -> c.copy(url = fileUri) }
+						chapters = info.chapters?.map { c -> c.copy(url = fileUri) },
 					)
 				}
 			}
@@ -288,7 +288,7 @@ class LocalMangaRepository(private val storageManager: LocalStorageManager) : Ma
 		locks.lock(id)
 	}
 
-	suspend fun unlockManga(id: Long) {
+	fun unlockManga(id: Long) {
 		locks.unlock(id)
 	}
 

app/src/main/java/org/koitharu/kotatsu/utils/CompositeMutex.kt

@@ -1,13 +1,13 @@
 package org.koitharu.kotatsu.utils
 
+import java.util.*
+import kotlin.coroutines.coroutineContext
+import kotlin.coroutines.resume
 import kotlinx.coroutines.CancellableContinuation
-import kotlinx.coroutines.currentCoroutineContext
 import kotlinx.coroutines.isActive
 import kotlinx.coroutines.suspendCancellableCoroutine
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
-import java.util.*
-import kotlin.coroutines.resume
 
 class CompositeMutex<T : Any> : Set<T> {
 
@@ -34,7 +34,7 @@ class CompositeMutex<T : Any> : Set<T> {
 	}
 
 	suspend fun lock(element: T) {
-		while (currentCoroutineContext().isActive) {
+		while (coroutineContext.isActive) {
 			waitForRemoval(element)
 			mutex.withLock {
 				if (data[element] == null) {
@@ -45,12 +45,10 @@ class CompositeMutex<T : Any> : Set<T> {
 		}
 	}
 
-	suspend fun unlock(element: T) {
+	fun unlock(element: T) {
-		val continuations = mutex.withLock {
+		val continuations = checkNotNull(data.remove(element)) {
-			checkNotNull(data.remove(element)) {
 			"CompositeMutex is not locked for $element"
 		}
-		}
 		continuations.forEach { c ->
 			if (c.isActive) {
 				c.resume(Unit)

app/src/main/java/org/koitharu/kotatsu/utils/LifecycleAwareServiceConnection.kt

@@ -1,45 +0,0 @@
-package org.koitharu.kotatsu.utils
-
-import android.app.Activity
-import android.content.ComponentName
-import android.content.Intent
-import android.content.ServiceConnection
-import android.os.IBinder
-import androidx.lifecycle.DefaultLifecycleObserver
-import androidx.lifecycle.LifecycleOwner
-import kotlinx.coroutines.flow.MutableStateFlow
-import kotlinx.coroutines.flow.StateFlow
-
-class LifecycleAwareServiceConnection(
-	private val host: Activity,
-) : ServiceConnection, DefaultLifecycleObserver {
-
-	private val serviceStateFlow = MutableStateFlow<IBinder?>(null)
-
-	val service: StateFlow<IBinder?>
-		get() = serviceStateFlow
-
-	override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
-		serviceStateFlow.value = service
-	}
-
-	override fun onServiceDisconnected(name: ComponentName?) {
-		serviceStateFlow.value = null
-	}
-
-	override fun onDestroy(owner: LifecycleOwner) {
-		super.onDestroy(owner)
-		host.unbindService(this)
-	}
-}
-
-fun Activity.bindServiceWithLifecycle(
-	owner: LifecycleOwner,
-	service: Intent,
-	flags: Int
-): LifecycleAwareServiceConnection {
-	val connection = LifecycleAwareServiceConnection(this)
-	bindService(service, connection, flags)
-	owner.lifecycle.addObserver(connection)
-	return connection
-}

app/src/main/res/layout/activity_downloads.xml

@@ -48,7 +48,6 @@
 		android:gravity="center"
 		android:text="@string/text_downloads_holder"
 		android:textAppearance="?attr/textAppearanceBody2"
-		android:visibility="gone"
 		tools:visibility="visible" />
 
 </androidx.coordinatorlayout.widget.CoordinatorLayout>

app/src/test/java/org/koitharu/kotatsu/utils/CompositeMutexTest.kt

@@ -1,17 +1,14 @@
 package org.koitharu.kotatsu.utils
 
-import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.*
-import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.runTest
-import kotlinx.coroutines.withTimeoutOrNull
-import kotlinx.coroutines.yield
 import org.junit.Assert.assertNull
 import org.junit.Test
 
 class CompositeMutexTest {
 
 	@Test
-	fun testSingleLock() = runTest {
+	fun singleLock() = runTest {
 		val mutex = CompositeMutex<Int>()
 		mutex.lock(1)
 		mutex.lock(2)
@@ -22,7 +19,7 @@ class CompositeMutexTest {
 	}
 
 	@Test
-	fun testDoubleLock() = runTest {
+	fun doubleLock() = runTest {
 		val mutex = CompositeMutex<Int>()
 		repeat(2) {
 			launch(Dispatchers.Default) {
@@ -36,4 +33,20 @@ class CompositeMutexTest {
 		}
 		assertNull(tryLock)
 	}
+
+	@Test
+	fun cancellation() = runTest {
+		val mutex = CompositeMutex<Int>()
+		mutex.lock(1)
+		val job = launch {
+			try {
+				mutex.lock(1)
+			} finally {
+				mutex.unlock(1)
+			}
+		}
+		withTimeout(2000) {
+			job.cancelAndJoin()
+		}
+	}
 }