From 4ef1f5c90f62fc9f03367d412b0fa0af4f8afda1 Mon Sep 17 00:00:00 2001 From: SpiritCroc Date: Sat, 10 Jul 2021 10:51:27 +0200 Subject: [PATCH 1/5] Avoid incomplete downloads in cache Previously, when a download was aborted (e.g. due to a bad internet connection), a partly downloaded file was remaining in cache, which would then be delivered upon later requests. This can lead e.g. to chats where images aren't loading. To avoid this, first download files to a temporary file that is not the final cache file, and only rename/move it on finish. Note that if you already have broken downloads, you still need to clear cache once to get rid of them after this commit, but it should not occur anymore afterwards. --- .../sdk/internal/session/DefaultFileService.kt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt index a284d976d0..ba32bca6d6 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt @@ -131,8 +131,14 @@ internal class DefaultFileService @Inject constructor( Timber.v("Response size ${response.body?.contentLength()} - Stream available: ${!source.exhausted()}") // Write the file to cache (encrypted version if the file is encrypted) - writeToFile(source.inputStream(), cachedFiles.file) + // Write to a tmp file first, so if we abort before done, we don't have a broken cached file + val tmpFile = File(cachedFiles.file.parentFile, "${cachedFiles.file.name}.tmp") + if (tmpFile.exists()) { + Timber.v("## FileService: discard aborted tmp file ${tmpFile.path}") + } + writeToFile(source.inputStream(), tmpFile) response.close() + tmpFile.renameTo(cachedFiles.file) } else { Timber.v("## FileService: cache hit for $url") } @@ -145,8 +151,13 @@ internal class DefaultFileService @Inject constructor( Timber.v("## FileService: decrypt file") // Ensure the parent folder exists cachedFiles.decryptedFile.parentFile?.mkdirs() + // Write to a tmp file first, so if we abort before done, we don't have a broken cached file + val tmpFile = File(cachedFiles.decryptedFile.parentFile, "${cachedFiles.decryptedFile.name}.tmp") + if (tmpFile.exists()) { + Timber.v("## FileService: discard aborted tmp file ${tmpFile.path}") + } val decryptSuccess = cachedFiles.file.inputStream().use { inputStream -> - cachedFiles.decryptedFile.outputStream().buffered().use { outputStream -> + tmpFile.outputStream().buffered().use { outputStream -> MXEncryptedAttachments.decryptAttachment( inputStream, elementToDecrypt, @@ -154,6 +165,7 @@ internal class DefaultFileService @Inject constructor( ) } } + tmpFile.renameTo(cachedFiles.decryptedFile) if (!decryptSuccess) { throw IllegalStateException("Decryption error") } From 512e1b339dcaef0c63a4d2a11de315725fc02c3d Mon Sep 17 00:00:00 2001 From: SpiritCroc Date: Sat, 10 Jul 2021 11:07:21 +0200 Subject: [PATCH 2/5] Add changelog.d/3656.bugfix --- changelog.d/3656.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3656.bugfix diff --git a/changelog.d/3656.bugfix b/changelog.d/3656.bugfix new file mode 100644 index 0000000000..30d451558f --- /dev/null +++ b/changelog.d/3656.bugfix @@ -0,0 +1 @@ +Avoid incomplete downloads in cache From 9c1bec94c9874e83233f283d2578ad9004d9431a Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 19 Jul 2021 10:57:35 +0200 Subject: [PATCH 3/5] Create AtomicFileCreator class to avoid code copy/paste --- .../internal/session/DefaultFileService.kt | 23 +++++------ .../internal/util/file/AtomicFileCreator.kt | 38 +++++++++++++++++++ 2 files changed, 47 insertions(+), 14 deletions(-) create mode 100644 matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt index ba32bca6d6..c118352527 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt @@ -33,6 +33,7 @@ import org.matrix.android.sdk.internal.di.SessionDownloadsDirectory import org.matrix.android.sdk.internal.di.UnauthenticatedWithCertificateWithProgress import org.matrix.android.sdk.internal.session.download.DownloadProgressInterceptor.Companion.DOWNLOAD_PROGRESS_INTERCEPTOR_HEADER import org.matrix.android.sdk.internal.util.MatrixCoroutineDispatchers +import org.matrix.android.sdk.internal.util.file.AtomicFileCreator import org.matrix.android.sdk.internal.util.md5 import org.matrix.android.sdk.internal.util.writeToFile import timber.log.Timber @@ -131,14 +132,11 @@ internal class DefaultFileService @Inject constructor( Timber.v("Response size ${response.body?.contentLength()} - Stream available: ${!source.exhausted()}") // Write the file to cache (encrypted version if the file is encrypted) - // Write to a tmp file first, so if we abort before done, we don't have a broken cached file - val tmpFile = File(cachedFiles.file.parentFile, "${cachedFiles.file.name}.tmp") - if (tmpFile.exists()) { - Timber.v("## FileService: discard aborted tmp file ${tmpFile.path}") - } - writeToFile(source.inputStream(), tmpFile) + // Write to a part file first, so if we abort before done, we don't have a broken cached file + val atomicFileCreator = AtomicFileCreator(cachedFiles.file) + writeToFile(source.inputStream(), atomicFileCreator.partFile) response.close() - tmpFile.renameTo(cachedFiles.file) + atomicFileCreator.commit() } else { Timber.v("## FileService: cache hit for $url") } @@ -151,13 +149,10 @@ internal class DefaultFileService @Inject constructor( Timber.v("## FileService: decrypt file") // Ensure the parent folder exists cachedFiles.decryptedFile.parentFile?.mkdirs() - // Write to a tmp file first, so if we abort before done, we don't have a broken cached file - val tmpFile = File(cachedFiles.decryptedFile.parentFile, "${cachedFiles.decryptedFile.name}.tmp") - if (tmpFile.exists()) { - Timber.v("## FileService: discard aborted tmp file ${tmpFile.path}") - } + // Write to a part file first, so if we abort before done, we don't have a broken cached file + val atomicFileCreator = AtomicFileCreator(cachedFiles.decryptedFile) val decryptSuccess = cachedFiles.file.inputStream().use { inputStream -> - tmpFile.outputStream().buffered().use { outputStream -> + atomicFileCreator.partFile.outputStream().buffered().use { outputStream -> MXEncryptedAttachments.decryptAttachment( inputStream, elementToDecrypt, @@ -165,7 +160,7 @@ internal class DefaultFileService @Inject constructor( ) } } - tmpFile.renameTo(cachedFiles.decryptedFile) + atomicFileCreator.commit() if (!decryptSuccess) { throw IllegalStateException("Decryption error") } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt new file mode 100644 index 0000000000..e24473db6d --- /dev/null +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2021 The Matrix.org Foundation C.I.C. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.matrix.android.sdk.internal.util.file + +import timber.log.Timber +import java.io.File + +internal class AtomicFileCreator(private val file: File) { + val partFile = File(file.parentFile, "${file.name}.part") + + init { + if (file.exists()) { + Timber.w("## AtomicFileCreator: target file ${file.path} exists, it should not happen.") + } + if (partFile.exists()) { + Timber.d("## AtomicFileCreator: discard aborted part file ${partFile.path}") + // No need to delete the file, we will overwrite it + } + } + + fun commit() { + partFile.renameTo(file) + } +} From 7643cc506d1143f07707311313f57e1d688ab578 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 19 Jul 2021 11:08:03 +0200 Subject: [PATCH 4/5] Remove part file(s) in case of failure Will not always delete part files in case of crashes --- .../sdk/internal/session/DefaultFileService.kt | 12 ++++++++++-- .../sdk/internal/util/file/AtomicFileCreator.kt | 4 ++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt index c118352527..efa5cbfb90 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt @@ -97,6 +97,9 @@ internal class DefaultFileService @Inject constructor( } } + var atomicFileCreator1: AtomicFileCreator? = null + var atomicFileCreator2: AtomicFileCreator? = null + if (existingDownload != null) { // FIXME If the first downloader cancels then we'll unfortunately be cancelled too. return existingDownload.await() @@ -133,7 +136,7 @@ internal class DefaultFileService @Inject constructor( // Write the file to cache (encrypted version if the file is encrypted) // Write to a part file first, so if we abort before done, we don't have a broken cached file - val atomicFileCreator = AtomicFileCreator(cachedFiles.file) + val atomicFileCreator = AtomicFileCreator(cachedFiles.file).also { atomicFileCreator1 = it } writeToFile(source.inputStream(), atomicFileCreator.partFile) response.close() atomicFileCreator.commit() @@ -150,7 +153,7 @@ internal class DefaultFileService @Inject constructor( // Ensure the parent folder exists cachedFiles.decryptedFile.parentFile?.mkdirs() // Write to a part file first, so if we abort before done, we don't have a broken cached file - val atomicFileCreator = AtomicFileCreator(cachedFiles.decryptedFile) + val atomicFileCreator = AtomicFileCreator(cachedFiles.decryptedFile).also { atomicFileCreator2 = it } val decryptSuccess = cachedFiles.file.inputStream().use { inputStream -> atomicFileCreator.partFile.outputStream().buffered().use { outputStream -> MXEncryptedAttachments.decryptAttachment( @@ -181,6 +184,11 @@ internal class DefaultFileService @Inject constructor( } toNotify?.completeWith(result) + result.onFailure { + atomicFileCreator1?.cancel() + atomicFileCreator2?.cancel() + } + return result.getOrThrow() } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt index e24473db6d..ca10c0ed0f 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/util/file/AtomicFileCreator.kt @@ -32,6 +32,10 @@ internal class AtomicFileCreator(private val file: File) { } } + fun cancel() { + partFile.delete() + } + fun commit() { partFile.renameTo(file) } From a2996ee042978012766a9f722fd6a25f5b1afd22 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 19 Jul 2021 16:32:02 +0200 Subject: [PATCH 5/5] Rename var --- .../sdk/internal/session/DefaultFileService.kt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt index efa5cbfb90..1cbf621d36 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/DefaultFileService.kt @@ -97,8 +97,8 @@ internal class DefaultFileService @Inject constructor( } } - var atomicFileCreator1: AtomicFileCreator? = null - var atomicFileCreator2: AtomicFileCreator? = null + var atomicFileDownload: AtomicFileCreator? = null + var atomicFileDecrypt: AtomicFileCreator? = null if (existingDownload != null) { // FIXME If the first downloader cancels then we'll unfortunately be cancelled too. @@ -136,7 +136,7 @@ internal class DefaultFileService @Inject constructor( // Write the file to cache (encrypted version if the file is encrypted) // Write to a part file first, so if we abort before done, we don't have a broken cached file - val atomicFileCreator = AtomicFileCreator(cachedFiles.file).also { atomicFileCreator1 = it } + val atomicFileCreator = AtomicFileCreator(cachedFiles.file).also { atomicFileDownload = it } writeToFile(source.inputStream(), atomicFileCreator.partFile) response.close() atomicFileCreator.commit() @@ -153,7 +153,7 @@ internal class DefaultFileService @Inject constructor( // Ensure the parent folder exists cachedFiles.decryptedFile.parentFile?.mkdirs() // Write to a part file first, so if we abort before done, we don't have a broken cached file - val atomicFileCreator = AtomicFileCreator(cachedFiles.decryptedFile).also { atomicFileCreator2 = it } + val atomicFileCreator = AtomicFileCreator(cachedFiles.decryptedFile).also { atomicFileDecrypt = it } val decryptSuccess = cachedFiles.file.inputStream().use { inputStream -> atomicFileCreator.partFile.outputStream().buffered().use { outputStream -> MXEncryptedAttachments.decryptAttachment( @@ -185,8 +185,8 @@ internal class DefaultFileService @Inject constructor( toNotify?.completeWith(result) result.onFailure { - atomicFileCreator1?.cancel() - atomicFileCreator2?.cancel() + atomicFileDownload?.cancel() + atomicFileDecrypt?.cancel() } return result.getOrThrow()