Skip to content

Mitigate race condition in network caching #299

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.jetpackduba.gitnuro.images

import com.jetpackduba.gitnuro.logging.printError

object InMemoryImagesCache : ImagesCache {
private val cachedImages = hashMapOf<String, ByteArray>()

Expand All @@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -14,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

Expand All @@ -30,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()
Expand All @@ -51,6 +65,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()
Expand Down Expand Up @@ -100,4 +118,8 @@ fun rememberNetworkImageOrNull(url: String, placeHolderImageRes: String? = null)
fun ByteArray.toComposeImage() = Image.makeFromEncoded(this).toComposeImageBitmap()


internal class ValueHolder<T>(var value: T)
internal class ValueHolder<T>(var value: T) {
override fun toString(): String {
return value.toString()
}
}