Skip to content

improve chunked-upload timeout #2130

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.

Sign up for GitHub

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

Open
alperozturk96 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

improve chunked-upload timeout #2130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a14fb7a
use more reasonable values for assemble
alperozturk96 Sep 1, 2026
2a8a435
use kotlin.time.Duration
alperozturk96 Sep 1, 2026
05e37ba
adjust values
alperozturk96 Sep 1, 2026
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
41 changes: 19 additions & 22 deletions ...rc/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
64 changes: 45 additions & 19 deletions ...est/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,

@tobiasKaminsky tobiasKaminsky Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

expected value should be hardcoded, else it does the same as calc function and thus if there is problem, it will not be seen?

@alperozturk96 alperozturk96 Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Already like that. I don't get what you mean.

Image

@tobiasKaminsky tobiasKaminsky Sep 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

x + y / 1000 is not hardcoded.
I would rather use 3, so that it does not depend on any calculation.

In your initial description you wrote:
min assemble time to 3 min

With "your" test it was wrong/not clear that there is a diff between code and desc.

So having real values, from

  • 3 minimum
  • 13 1Gb
  • 26 2Gb
  • 60 maximum

makes it easier to read, and also reveals any logic error, if computation of it changes?

@alperozturk96 alperozturk96 Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I got what you mean but having magic numbers in tests makes the test more unclear to me.

These are ASSEMBLE_TIME_BASE, ASSEMBLE_TIME_PER_GB constants. If we change those also magic numbers in the tests needs to be changed.

Those constants part of the production logic. Purpose of the assert is here are we getting expected duration against the calculated assemble time out.

e.g. for 1GB file -> should be ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB and calculateAssembleTimeout must return same value.

ASSEMBLE_TIME_BASE is 1.minutes
ASSEMBLE_TIME_PER_GB is 10.minutes
ASSEMBLE_TIME_MAX is 1.hour

so 11.minutes * 1 = 11 and min of 11.minutes and 1.hour is 11.minutes why we should write -> 11 * 60 * 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
Expand Down
Loading