From 01fdc6d1bb5690004b6796859b085cc21e49b381 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 25 Jun 2020 12:14:17 +0200 Subject: [PATCH] Avoid downloading same file several times --- .../android/api/session/file/FileService.kt | 8 +++ .../internal/session/DefaultFileService.kt | 71 +++++++++++++++++-- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/file/FileService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/file/FileService.kt index d566de475a..a6f9f1b078 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/file/FileService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/api/session/file/FileService.kt @@ -42,6 +42,12 @@ interface FileService { FOR_EXTERNAL_SHARE } + enum class FileState { + IN_CACHE, + DOWNLOADING, + UNKNOWN + } + /** * Download a file. * Result will be a decrypted file, stored in the cache folder. id parameter will be used to create a sub folder to avoid name collision. @@ -63,4 +69,6 @@ interface FileService { * (if not other app won't be able to access it) */ fun getTemporarySharableURI(mxcUrl: String, mimeType: String?): Uri? + + fun fileState(mxcUrl: String, mimeType: String?) : FileState } diff --git a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/DefaultFileService.kt b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/DefaultFileService.kt index 2d6e955cbb..3be7ce63bd 100644 --- a/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/DefaultFileService.kt +++ b/matrix-sdk-android/src/main/java/im/vector/matrix/android/internal/session/DefaultFileService.kt @@ -22,16 +22,17 @@ import android.webkit.MimeTypeMap import androidx.core.content.FileProvider import arrow.core.Try import im.vector.matrix.android.api.MatrixCallback +import im.vector.matrix.android.api.extensions.tryThis import im.vector.matrix.android.api.session.content.ContentUrlResolver import im.vector.matrix.android.api.session.file.FileService import im.vector.matrix.android.api.util.Cancelable +import im.vector.matrix.android.api.util.NoOpCancellable import im.vector.matrix.android.internal.crypto.attachments.ElementToDecrypt import im.vector.matrix.android.internal.crypto.attachments.MXEncryptedAttachments import im.vector.matrix.android.internal.di.CacheDirectory import im.vector.matrix.android.internal.di.ExternalFilesDirectory import im.vector.matrix.android.internal.di.SessionDownloadsDirectory import im.vector.matrix.android.internal.di.WithProgress -import im.vector.matrix.android.internal.extensions.foldToCallback import im.vector.matrix.android.internal.task.TaskExecutor import im.vector.matrix.android.internal.util.MatrixCoroutineDispatchers import im.vector.matrix.android.internal.util.toCancelable @@ -65,9 +66,14 @@ internal class DefaultFileService @Inject constructor( private val downloadFolder = File(sessionCacheDirectory, "MF") + /** + * Retain ongoing downloads to avoid re-downloading and already downloading file + * map of mxCurl to callbacks + */ + private val ongoing = mutableMapOf>>() + /** * Download file in the cache folder, and eventually decrypt it - * TODO implement clear file, to delete "MF" * TODO looks like files are copied 3 times */ override fun downloadFile(downloadMode: FileService.DownloadMode, @@ -77,10 +83,29 @@ internal class DefaultFileService @Inject constructor( url: String?, elementToDecrypt: ElementToDecrypt?, callback: MatrixCallback): Cancelable { + + val unwrappedUrl = url ?: return NoOpCancellable.also { + callback.onFailure(IllegalArgumentException("url is null")) + } + + Timber.v("## FileService downloadFile $unwrappedUrl") + + synchronized(ongoing) { + val existing = ongoing[unwrappedUrl] + if (existing != null) { + Timber.v("## FileService downloadFile is already downloading.. ") + existing.add(callback) + return NoOpCancellable + } else { + // mark as tracked + ongoing[unwrappedUrl] = ArrayList() + // and proceed to download + } + } + return taskExecutor.executorScope.launch(coroutineDispatchers.main) { withContext(coroutineDispatchers.io) { Try { - val unwrappedUrl = url ?: throw IllegalArgumentException("url is null") if (!downloadFolder.exists()) { downloadFolder.mkdirs() } @@ -94,7 +119,7 @@ internal class DefaultFileService @Inject constructor( val request = Request.Builder() .url(resolvedUrl) - .header("matrix-sdk:mxc_URL", url ?: "") + .header("matrix-sdk:mxc_URL", url) .build() val response = try { @@ -131,8 +156,31 @@ internal class DefaultFileService @Inject constructor( Try.just(copyFile(destFile, downloadMode)) } - } - .foldToCallback(callback) + }.fold({ + callback.onFailure(it) + // notify concurrent requests + val toNotify = synchronized(ongoing) { + ongoing[unwrappedUrl]?.also { + ongoing.remove(unwrappedUrl) + } + } + toNotify?.forEach { otherCallbacks -> + tryThis { otherCallbacks.onFailure(it) } + } + }, { file -> + callback.onSuccess(file) + // notify concurrent requests + val toNotify = synchronized(ongoing) { + ongoing[unwrappedUrl]?.also { + ongoing.remove(unwrappedUrl) + } + } + Timber.v("## FileService additional to notify ${toNotify?.size ?: 0} ") + toNotify?.forEach { otherCallbacks -> + tryThis { otherCallbacks.onSuccess(file) } + } + }) + }.toCancelable() } @@ -142,7 +190,15 @@ internal class DefaultFileService @Inject constructor( } override fun isFileInCache(mxcUrl: String, mimeType: String?): Boolean { - return File(downloadFolder, fileForUrl(mxcUrl, mimeType)).exists() + return File(downloadFolder, fileForUrl(mxcUrl, mimeType)).exists() + } + + override fun fileState(mxcUrl: String, mimeType: String?): FileService.FileState { + if (isFileInCache(mxcUrl,mimeType)) return FileService.FileState.IN_CACHE + val isDownloading = synchronized(ongoing) { + ongoing[mxcUrl] != null + } + return if (isDownloading) FileService.FileState.DOWNLOADING else FileService.FileState.UNKNOWN } /** @@ -158,6 +214,7 @@ internal class DefaultFileService @Inject constructor( } private fun copyFile(file: File, downloadMode: FileService.DownloadMode): File { + // TODO some of this seems outdated, will need to be re-worked return when (downloadMode) { FileService.DownloadMode.TO_EXPORT -> file.copyTo(File(externalFilesDirectory, file.name), true)