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.

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

Make the DataOkHttpUploader state volatile #2305

Merged
merged 1 commit into from
Oct 4, 2024
+45 −1
Merged

Make the DataOkHttpUploader state volatile #2305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion ...-core/src/main/kotlin/com/datadog/android/core/internal/data/upload/DataOkHttpUploader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,13 @@ internal class DataOkHttpUploader(
val androidInfoProvider: AndroidInfoProvider
) : DataUploader {

private var attempts = 1
@Volatile
private var attempts: Int = 1
Copy link
Member

Choose a reason for hiding this comment

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

why not making this one as @Volatile or even AtomicInteger. There is no concurrency right now (so probably AtomicInteger is not quite useful), but for the changes at least shouldn't we declare volatile access?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was almost convinced that primitives are volatile in JVM. The docs are saying either true or false so let's do this also Volatile. There is no need for Atomicity there.

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html

Reads and writes are atomic for reference variables and for most primitive variables (all types except long and double).
Reads and writes are atomic for all variables declared volatile (including long and double variables).

I'm not sure about ART though, but maybe it should maintain this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Also atomic read/write is different from cross-thread visibility though.

When a field is declared volatile, the compiler and runtime are put on notice that this variable is shared and that operations on it should not be reordered with other memory operations. Volatile variables are not cached in registers or in caches where they are hidden from other processors, so a read of a volatile variable always returns the most recent write by any thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes there are 2 different things but I am pretty sure I've read also about volatility once, anyway I guess without going into the bytecode you will never now for sure.


@Volatile
private var previousUploadStatus: UploadStatus? = null

@Volatile
private var previousUploadedBatchId: BatchId? = null

// region DataUploader
Expand Down
39 changes: 39 additions & 0 deletions ...e/src/test/kotlin/com/datadog/android/core/internal/data/upload/DataOkHttpUploaderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,45 @@ internal class DataOkHttpUploaderTest {
}
}

@Test
fun `M pass the ExecutionContext to requestFactory W upload { same batchId retried, new thread each time }`(
// we are testing the same as the previous test, but we are making sure that the state of the uploader
// it is shared between threads (it should be kept in the heap and not thread local memory)
@Forgery batch: List<RawBatchEvent>,
@StringForgery batchMeta: String,
@StringForgery message: String,
@IntForgery(2, 10) retries: Int,
@Forgery batchId: BatchId,
forge: Forge
) {
// Given
val statusCodes = forge.aList(size = retries) { forge.anInt(400, 600) }
whenever(mockCall.execute()) doReturnConsecutively statusCodes.map { mockResponse(it, message) }

// When
repeat(retries) {
val thread = Thread {
testedUploader.upload(fakeContext, batch, batchMeta.toByteArray(), batchId)
}
thread.start()
thread.join()
}

// Then
argumentCaptor<RequestExecutionContext>() {
verify(mockRequestFactory, times(retries))
.create(eq(fakeContext), capture(), eq(batch), eq(batchMeta.toByteArray()))
allValues.forEachIndexed() { index, value ->
assertThat(value.attemptNumber).isEqualTo(index + 1)
if (index == 0) {
assertThat(value.previousResponseCode).isNull()
} else {
assertThat(value.previousResponseCode).isEqualTo(statusCodes[index - 1])
}
}
}
}

@Test
fun `M pass empty ExecutionContext to requestFactory W upload { null batchId }`(
@Forgery batch: List<RawBatchEvent>,
Expand Down