Skip to content
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

Complement for #3656 #3699

Merged
merged 6 commits into from
Jul 19, 2021
Merged
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
1 change: 1 addition & 0 deletions changelog.d/3656.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Avoid incomplete downloads in cache
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -96,6 +97,9 @@ internal class DefaultFileService @Inject constructor(
}
}

var atomicFileDownload: AtomicFileCreator? = null
var atomicFileDecrypt: AtomicFileCreator? = null

if (existingDownload != null) {
// FIXME If the first downloader cancels then we'll unfortunately be cancelled too.
return existingDownload.await()
Expand Down Expand Up @@ -131,8 +135,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)
writeToFile(source.inputStream(), cachedFiles.file)
// 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 { atomicFileDownload = it }
writeToFile(source.inputStream(), atomicFileCreator.partFile)
response.close()
atomicFileCreator.commit()
} else {
Timber.v("## FileService: cache hit for $url")
}
Expand All @@ -145,15 +152,18 @@ internal class DefaultFileService @Inject constructor(
Timber.v("## FileService: decrypt file")
// 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 { atomicFileDecrypt = it }
val decryptSuccess = cachedFiles.file.inputStream().use { inputStream ->
cachedFiles.decryptedFile.outputStream().buffered().use { outputStream ->
atomicFileCreator.partFile.outputStream().buffered().use { outputStream ->
MXEncryptedAttachments.decryptAttachment(
inputStream,
elementToDecrypt,
outputStream
)
}
}
atomicFileCreator.commit()
if (!decryptSuccess) {
throw IllegalStateException("Decryption error")
}
Expand All @@ -174,6 +184,11 @@ internal class DefaultFileService @Inject constructor(
}
toNotify?.completeWith(result)

result.onFailure {
atomicFileDownload?.cancel()
atomicFileDecrypt?.cancel()
}

return result.getOrThrow()
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* 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 cancel() {
partFile.delete()
}

fun commit() {
partFile.renameTo(file)
}
}