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

Jump to bottom

RUM-6307 Make sure ConsentAwareFileOrchestrator is thread safe #2309

Closed
wants to merge 1 commit into from
Closed

RUM-6307 Make sure ConsentAwareFileOrchestrator is thread safe #2309

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
1 change: 1 addition & 0 deletions ...m/datadog/android/core/internal/persistence/file/advanced/ConsentAwareFileOrchestrator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ internal open class ConsentAwareFileOrchestrator(
internal val internalLogger: InternalLogger
) : FileOrchestrator, TrackingConsentProviderCallback {

@Volatile
private lateinit var delegateOrchestrator: FileOrchestrator

init {
Expand Down
46 changes: 46 additions & 0 deletions ...-it/src/androidTest/kotlin/com/datadog/android/core/integration/tests/FeatureScopeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit

/**
* Instrumentation tests for the feature scope.
Expand Down Expand Up @@ -156,6 +158,50 @@ class FeatureScopeTest : MockServerTest() {
}.doWait(LONG_WAIT_MS)
}

@Test
fun mustReceiveTheEvents_whenFeatureWrite_trackingConsentPendingToGranted_switched_asynchronously() {
jonathanmos marked this conversation as resolved.
Show resolved Hide resolved
ambushwork marked this conversation as resolved.
Show resolved Hide resolved
// Given
trackingConsent = TrackingConsent.PENDING
testedInternalSdkCore = Datadog.initialize(
context = ApplicationProvider.getApplicationContext(),
configuration = fakeConfiguration,
trackingConsent = trackingConsent
) as InternalSdkCore
testedInternalSdkCore.registerFeature(stubFeature)
val featureScope = testedInternalSdkCore.getFeature(fakeFeatureName)
checkNotNull(featureScope)
val countDownLatch = CountDownLatch(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a problem here: CountDownLatch is initialized with value 1, and it will be set to 0 after the first batch write, and once all the batches written, value will be negative.

It means, that countDownLatch.await(TimeUnit.SECONDS.toMillis(5), TimeUnit.MILLISECONDS) will be unblocked when first batch is written, not when all batches are written.

We won't notice an issue, because we also have ConditionWatcher which wait for the data, but I suppose if the original intention was "write at least one batch in pending mode and then continue with tracking consent switch", then this await should be moved inside:

 val switchConsentThread = Thread {
    Datadog.setTrackingConsent(TrackingConsent.GRANTED)
}

Probably the switchConsentThread.join() is not needed as well, if we are relying on the waiting ConditionWatcher (but having it doesn't hurt).

Thread {
fakeBatchData.forEach { rawBatchEvent ->
featureScope.withWriteContext { _, eventBatchWriter ->
eventBatchWriter.write(
rawBatchEvent,
fakeBatchMetadata,
eventType
)
countDownLatch.countDown()
}
}
}.start()

// When
val switchConsentThread = Thread {
Datadog.setTrackingConsent(TrackingConsent.GRANTED)
}
switchConsentThread.start()
switchConsentThread.join()

// Then
countDownLatch.await(TimeUnit.SECONDS.toMillis(5), TimeUnit.MILLISECONDS)
ConditionWatcher {
MockWebServerAssert.assertThat(getMockServerWrapper())
.withConfiguration(fakeConfiguration)
.withTrackingConsent(TrackingConsent.GRANTED)
.receivedData(fakeBatchData, fakeBatchMetadata)
true
}.doWait(LONG_WAIT_MS)
}

@Test
fun mustReceiveTheEvents_whenFeatureWrite_trackingConsentGranted_metadataIsNull() {
// Given
Expand Down