From db6cd64c787d687eca63d053517daa82d3dccd03 Mon Sep 17 00:00:00 2001 From: Jules Kerssemakers Date: Tue, 27 May 2025 23:27:37 +0200 Subject: [PATCH 1/2] Log whenever we load network images. Users deserve to know when their software is doing network access. Additionally, makes it easier for us developers to verify if cache works. --- .../com/jetpackduba/gitnuro/images/NetworkImageLoader.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt b/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt index 5b7b73bc..b0c71bcb 100644 --- a/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt +++ b/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt @@ -6,6 +6,7 @@ import androidx.compose.ui.graphics.toComposeImageBitmap import androidx.compose.ui.res.useResource import com.jetpackduba.gitnuro.extensions.acquireAndUse import com.jetpackduba.gitnuro.extensions.toByteArray +import com.jetpackduba.gitnuro.logging.printLog import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.sync.Semaphore import kotlinx.coroutines.withContext @@ -51,6 +52,10 @@ object NetworkImageLoader { } suspend fun loadImage(link: String): ByteArray = withContext(Dispatchers.IO) { + // Network access can be considered somewhat surprising for a repository-browser + // Don't be shy about admitting the work we do. + printLog("NET", "loading image from $link") + val url = URL(link) val connection = url.openConnection() as HttpURLConnection connection.connect() From acb70fcab69af4fccec2c22584e5f8957450d82b Mon Sep 17 00:00:00 2001 From: Jules Kerssemakers Date: Wed, 28 May 2025 00:06:17 +0200 Subject: [PATCH 2/2] Mitigate Re-entrant race condition in NetworkImageLoader: images loaded repeatedly from network. There's a race condition in the NetworkImageLoader / InMemoryImagesCache. Since image loading is done async, the UI will queue up the multiple commits while the first image is still streaming in from the net. If the next commit(s) is (are) from the same author, this will lead to a cache-miss (previous fetch isn't yet finished, thus not yet cached), which queues up ANOTHER network-fetch. This is a classic footgun in async code, going all the way back to java 1.0 with `synchronized`. To mitigate: do a Double-checked locking: retest cache *inside* the semaphore critical section. Even then, reduce the semaphore-count to 1; Any higher will lead to the critical section being executed multiple times in parallel, which means the retry will *still* race with itself in a parallel execution. A proper fix will require smarter locking, involving URL-specific locks. (probably something with concurrent maps containing futures?) See also: https://en.wikipedia.org/wiki/Double-checked_locking --- .../gitnuro/images/InMemoryImagesCache.kt | 5 ++++ .../gitnuro/images/NetworkImageLoader.kt | 25 ++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/main/kotlin/com/jetpackduba/gitnuro/images/InMemoryImagesCache.kt b/src/main/kotlin/com/jetpackduba/gitnuro/images/InMemoryImagesCache.kt index ee60e206..2cf272f8 100644 --- a/src/main/kotlin/com/jetpackduba/gitnuro/images/InMemoryImagesCache.kt +++ b/src/main/kotlin/com/jetpackduba/gitnuro/images/InMemoryImagesCache.kt @@ -1,5 +1,7 @@ package com.jetpackduba.gitnuro.images +import com.jetpackduba.gitnuro.logging.printError + object InMemoryImagesCache : ImagesCache { private val cachedImages = hashMapOf() @@ -8,6 +10,9 @@ object InMemoryImagesCache : ImagesCache { } override fun cacheImage(urlSource: String, image: ByteArray) { + if (cachedImages[urlSource] != null) { + printError("CACHE", "Race condition! Image for $urlSource was already in cache") + } cachedImages[urlSource] = image } } \ No newline at end of file diff --git a/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt b/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt index b0c71bcb..c0d63694 100644 --- a/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt +++ b/src/main/kotlin/com/jetpackduba/gitnuro/images/NetworkImageLoader.kt @@ -15,9 +15,13 @@ import java.io.FileNotFoundException import java.net.HttpURLConnection import java.net.URL -private const val MAX_LOADING_IMAGES = 3 - object NetworkImageLoader { + // FIXME: Keep this at 1, until we fix locking per-URL + // Above 1, the URL-fetch in the critical section will race with itself, + // because multiple fetches will be queued while the first image is still being put + // into the cache. + private const val MAX_LOADING_IMAGES = 1 + private val loadingImagesSemaphore = Semaphore(MAX_LOADING_IMAGES) private val cache: ImagesCache = InMemoryImagesCache @@ -31,10 +35,19 @@ object NetworkImageLoader { try { val cachedImage = loadCachedImage(url) - if (cachedImage != null) + if (cachedImage != null) { return@withContext cachedImage + } loadingImagesSemaphore.acquireAndUse { + // re-try cache; perhaps another coroutine found the same image + // while we were waiting to acquire the semaphore + val retryCachedImage = loadCachedImage(url) + if (retryCachedImage != null) { + printLog("NET", "found image $url on retry"); + return@withContext retryCachedImage + } + val imageByteArray = loadImage(url) cache.cacheImage(url, imageByteArray) return@withContext imageByteArray.toComposeImage() @@ -105,4 +118,8 @@ fun rememberNetworkImageOrNull(url: String, placeHolderImageRes: String? = null) fun ByteArray.toComposeImage() = Image.makeFromEncoded(this).toComposeImageBitmap() -internal class ValueHolder(var value: T) \ No newline at end of file +internal class ValueHolder(var value: T) { + override fun toString(): String { + return value.toString() + } +} \ No newline at end of file