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

Avoid incomplete downloads in cache #3656

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

SpiritCroc
Copy link
Contributor

Pull Request Checklist

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.

Signed-off-by: Tobias Büttner [email protected]

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.
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, LGTM.
One small remark on the form.

@@ -145,15 +151,21 @@ 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a helper to avoid repeated the same code twice?
For instance

internal class AtomicFileCreator(private val file: File) {
    val tmpFile = File(file.parentFile, "${file.name}.tmp")

    init {
        if (tmpFile.exists()) {
            Timber.v("## AtomicFileCreator: discard aborted tmp file ${tmpFile.path}")
        }
    }

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

@bmarty bmarty changed the base branch from develop to feature/bma/part_dl July 19, 2021 08:50
@bmarty bmarty merged commit 2e64f89 into element-hq:feature/bma/part_dl Jul 19, 2021
@bmarty bmarty mentioned this pull request Jul 19, 2021
bmarty added a commit that referenced this pull request Jul 19, 2021
@SpiritCroc SpiritCroc deleted the broken_downloads branch July 20, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants