diff --git a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt index 34744a744..3c3eb382d 100644 --- a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt +++ b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt @@ -32,7 +32,10 @@ import java.io.RandomAccessFile import java.nio.channels.FileChannel import java.util.Locale import kotlin.math.max -import kotlin.math.min +import kotlin.time.Duration +import kotlin.time.Duration.Companion.hours +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.minutes @Suppress("LongParameterList") class ChunkedFileUploadRemoteOperation @@ -58,19 +61,6 @@ class ChunkedFileUploadRemoteOperation token, disableRetries ) { - // Assemble timeouts, in milliseconds. The literals are the definition itself, hence the MagicNumber opt-out. - @Suppress("MagicNumber") - @JvmField - val assembleTimeMin: Int = 30 * 1000 // 30s - - @Suppress("MagicNumber") - @JvmField - val assembleTimeMax: Int = 30 * 60 * 1000 // 30min - - @Suppress("MagicNumber") - @JvmField - val assembleTimePerGB: Int = 3 * 60 * 1000 // 3 min - private lateinit var uploadFolderUri: String private lateinit var destinationUri: String private var moveMethod: MoveMethod? = null @@ -215,7 +205,8 @@ class ChunkedFileUploadRemoteOperation creationTimestamp?.takeIf { it > 0 }?.let { move.addRequestHeader(OC_X_OC_CTIME_HEADER, it.toString()) } token?.let { move.addRequestHeader(E2E_TOKEN, it) } - val status = client.executeMethod(move, calculateAssembleTimeout(file), DO_NOT_CHANGE_DEFAULT) + val readTimeout = calculateAssembleTimeout(file).inWholeMilliseconds.toInt() + val status = client.executeMethod(move, readTimeout, DO_NOT_CHANGE_DEFAULT) return RemoteOperationResult(isSuccess(status), move) } @@ -299,19 +290,25 @@ class ChunkedFileUploadRemoteOperation } } - @VisibleForTesting - fun calculateAssembleTimeout(file: File): Int { - val fileSizeInGb = file.length() / BYTES_PER_GB - - return max(assembleTimeMin, min((assembleTimePerGB * fileSizeInGb).toInt(), assembleTimeMax)) - } - private data class UploadedChunks( val nextByte: Long, val lastId: Int ) companion object { + val ASSEMBLE_TIME_BASE: Duration = 1.minutes + + val ASSEMBLE_TIME_PER_GB: Duration = 10.minutes + + val ASSEMBLE_TIME_MAX: Duration = 1.hours + + @VisibleForTesting + fun calculateAssembleTimeout(file: File): Duration { + val fileSizeInGb = file.length() / BYTES_PER_GB + val timeout = minOf(ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * fileSizeInGb, ASSEMBLE_TIME_MAX) + return timeout.inWholeMilliseconds.milliseconds + } + const val MIN_CHUNK_SIZE: Long = 10240000 const val DEFAULT_CHUNK_SIZE: Long = 40960000 const val SERVER_MAX_CHUNK_SIZE_UNKNOWN: Long = -1 diff --git a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt index 4ce08c368..e650f101b 100644 --- a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt +++ b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt @@ -8,6 +8,9 @@ */ package com.owncloud.android.lib.resources.files +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_BASE +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_MAX +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_PER_GB import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals import org.junit.Test @@ -24,51 +27,74 @@ class ChunkedFileUploadRemoteOperationTest { @Test fun testAssembleTimeout() { MockitoAnnotations.openMocks(this) - val sut = - ChunkedFileUploadRemoteOperation( - null, - null, - null, - null, - System.currentTimeMillis() / 1000, - false - ) // 0b Mockito.`when`(file.length()).thenReturn(0L) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_BASE, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 100b Mockito.`when`(file.length()).thenReturn(100L) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_BASE, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 1Mb Mockito.`when`(file.length()).thenReturn(1 * MB) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB / 1000, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) - // 100Mb + // 100Mb, the size that used to be capped to the flat minimum Mockito.`when`(file.length()).thenReturn(100 * MB) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB / 10, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 1Gb Mockito.`when`(file.length()).thenReturn(1 * GB) - assertEquals(sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 2Gb Mockito.`when`(file.length()).thenReturn(2 * GB) - assertEquals((2 * sut.assembleTimePerGB), sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * 2, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 5Gb Mockito.`when`(file.length()).thenReturn(5 * GB) - assertEquals((5 * sut.assembleTimePerGB), sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * 5, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) + + Mockito.`when`(file.length()).thenReturn(6 * GB) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 50Gb Mockito.`when`(file.length()).thenReturn(50 * GB) - assertEquals(sut.assembleTimeMax, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 500Gb Mockito.`when`(file.length()).thenReturn(500 * GB) - assertEquals(sut.assembleTimeMax, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) + } + + @Test + fun assembleTimeoutGrowsWithFileSize() { + MockitoAnnotations.openMocks(this) + + val timeouts = + listOf(1 * MB, 100 * MB, 500 * MB, 1 * GB, 5 * GB).map { length -> + Mockito.`when`(file.length()).thenReturn(length) + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + } + + assertEquals(timeouts.sorted(), timeouts) + assertEquals(timeouts.distinct().size, timeouts.size) } @Test