Skip to content

Commit

Permalink
Merge branch 'hotfix/1.3.1'
Browse files Browse the repository at this point in the history
  • Loading branch information
@mariusc83
mariusc83 committed May 4, 2020
2 parents e8efc45 + 0adeffe commit 0f16d39
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 50 deletions.
11 changes: 4 additions & 7 deletions buildSrc/src/main/kotlin/com/datadog/gradle/plugin/apisurface/CheckApiSurfaceTask.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,13 @@ open class CheckApiSurfaceTask : DefaultTask() {
"git", "diff", "--color=never", "HEAD", "--", surfaceFile.absolutePath
)

val additions = lines.count { it.matches(Regex("^\\+[^+].*$")) }
val removals = lines.count { it.matches(Regex("^-[^-].*$")) }

if (additions > 0 || removals > 0) {
val additions = lines.filter { it.matches(Regex("^\\+[^+].*$")) }
val removals = lines.filter { it.matches(Regex("^-[^-].*$")) }

if (additions.isNotEmpty() || removals.isNotEmpty()) {
throw IllegalStateException(
"Make sure you run the ${ApiSurfacePlugin.TASK_GEN_API_SURFACE} task before you push your PR.\n" +
"---------\n" +
lines.joinToString("\n") +
"\n---------\n"
additions.joinToString("\n") + removals.joinToString("\n")
)
}
}
Expand Down
59 changes: 35 additions & 24 deletions dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/data/file/FileReader.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,12 @@ internal class FileReader(
private val suffix: CharSequence = ""
) : Reader {

private val readFiles: MutableSet<String> = mutableSetOf()
private val sentBatches: MutableSet<String> = mutableSetOf()
private val lockedFiles: MutableSet<String> = mutableSetOf()

// region LogReader

override fun readNextBatch(): Batch? {
val (file, data) = synchronized(readFiles) {
readNextFile()
}
val (file, data) = readNextFile()

return if (file == null) {
null
Expand All @@ -43,66 +40,80 @@ internal class FileReader(

override fun releaseBatch(batchId: String) {
sdkLogger.i("releaseBatch $batchId")
synchronized(readFiles) {
readFiles.remove(batchId)
synchronized(lockedFiles) {
lockedFiles.remove(batchId)
}
}

override fun dropBatch(batchId: String) {
sdkLogger.i("dropBatch $batchId")
sentBatches.add(batchId)
readFiles.remove(batchId)
val fileToDelete = File(dataDirectory, batchId)

deleteFile(fileToDelete)
if (deleteFile(fileToDelete)) {
releaseBatch(batchId)
}
}

override fun dropAllBatches() {
sdkLogger.i("dropAllBatches")
fileOrchestrator.getAllFiles().forEach { deleteFile(it) }
fileOrchestrator
.getAllFiles()
.forEach {
if (deleteFile(it)) {
releaseBatch(it.name)
}
}
}

// endregion

// region Internal

private fun readNextFile(): Pair<File?, ByteArray> {
val file = lockAndGetFile()
return if (file != null) {
file to file.readBytes(withPrefix = prefix, withSuffix = suffix)
} else {
file to ByteArray(0)
}
}

private fun lockAndGetFile(): File? {
var file: File? = null
val data = try {
file = fileOrchestrator.getReadableFile(sentBatches.union(readFiles))
if (file == null) {
ByteArray(0)
} else {
readFiles.add(file.name)
file.readBytes(withPrefix = prefix, withSuffix = suffix)
try {
synchronized(lockedFiles) {
val readFile = fileOrchestrator.getReadableFile(lockedFiles.toSet())
if (readFile != null) {
lockedFiles.add(readFile.name)
}
file = readFile
}
} catch (e: FileNotFoundException) {
sdkLogger.e("Couldn't create an input stream from file ${file?.path}", e)
ByteArray(0)
} catch (e: IOException) {
sdkLogger.e("Couldn't read messages from file ${file?.path}", e)
ByteArray(0)
} catch (e: SecurityException) {
sdkLogger.e("Couldn't access file ${file?.path}", e)
ByteArray(0)
} catch (e: OutOfMemoryError) {
sdkLogger.e("Couldn't read file ${file?.path} (not enough memory)", e)
ByteArray(0)
}

return file to data
return file
}

private fun deleteFile(fileToDelete: File) {
private fun deleteFile(fileToDelete: File): Boolean {
if (fileToDelete.exists()) {
if (fileToDelete.delete()) {
sdkLogger.d("File ${fileToDelete.path} deleted")
return true
} else {
sdkLogger.e("Error deleting file ${fileToDelete.path}")
}
} else {
sdkLogger.w("file ${fileToDelete.path} does not exist")
}

return false
}

// endregion
Expand Down
152 changes: 139 additions & 13 deletions dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/data/file/FileReaderTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.datadog.android.core.internal.data.file

import android.os.Build
import com.datadog.android.core.internal.data.Orchestrator
import com.datadog.android.core.internal.domain.Batch
import com.datadog.android.utils.forge.Configurator
import com.datadog.tools.unit.annotations.TestTargetApi
import com.datadog.tools.unit.extensions.ApiLevelExtension
Expand All @@ -16,6 +17,8 @@ import fr.xgouchet.elmyr.Forge
import fr.xgouchet.elmyr.junit5.ForgeConfiguration
import fr.xgouchet.elmyr.junit5.ForgeExtension
import java.io.File
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
Expand All @@ -25,14 +28,15 @@ import org.junit.jupiter.api.io.TempDir
import org.mockito.Mock
import org.mockito.junit.jupiter.MockitoExtension
import org.mockito.junit.jupiter.MockitoSettings
import org.mockito.quality.Strictness

@Extensions(
ExtendWith(MockitoExtension::class),
ExtendWith(ForgeExtension::class),
ExtendWith(ApiLevelExtension::class)
)
@ForgeConfiguration(Configurator::class)
@MockitoSettings()
@MockitoSettings(strictness = Strictness.LENIENT)
internal class FileReaderTest {

lateinit var testedReader: FileReader
Expand Down Expand Up @@ -85,8 +89,8 @@ internal class FileReaderTest {
val file = generateFile(fileName)
val data = forge.anAlphabeticalString()
file.writeText(data)
whenever(mockOrchestrator.getReadableFile(any())) doReturn null
whenever(mockOrchestrator.getReadableFile(emptySet())) doReturn file
whenever(mockOrchestrator.getReadableFile(mutableSetOf())) doReturn file
whenever(mockOrchestrator.getReadableFile(mutableSetOf(fileName))) doReturn null

val firstBatch = testedReader.readNextBatch()
val secondBatch = testedReader.readNextBatch()
Expand Down Expand Up @@ -187,15 +191,17 @@ internal class FileReaderTest {
) {
// given
val fileName = forge.anAlphabeticalString()
generateFile(fileName)
val file: File = generateFile(fileName)
whenever(mockOrchestrator.getReadableFile(any())).thenReturn(file)
testedReader.readNextBatch()

// when
// then
testedReader.dropBatch(fileName)

// then
val sentBatches: MutableSet<String> = testedReader.getFieldValue("sentBatches")
val lockedFiles: MutableSet<String> = testedReader.getFieldValue("lockedFiles")
assertThat(lockedFiles).isEmpty()
assertThat(rootDir.listFiles()).isEmpty()
assertThat(sentBatches).contains(fileName)
}

@Test
Expand All @@ -204,14 +210,17 @@ internal class FileReaderTest {
) {
// given
val fileName = forge.anAlphabeticalString()
val file: File = generateFile(fileName)
whenever(mockOrchestrator.getReadableFile(any())).thenReturn(file)
testedReader.readNextBatch()
val notExistingFileName = forge.anAlphabeticalString()

// when
testedReader.dropBatch(fileName)
testedReader.dropBatch(notExistingFileName)

// then
val sentBatches: MutableSet<String> = testedReader.getFieldValue("sentBatches")
assertThat(rootDir.listFiles()).isEmpty()
assertThat(sentBatches).contains(fileName)
val lockedFiles: MutableSet<String> = testedReader.getFieldValue("lockedFiles")
assertThat(lockedFiles).containsOnly(fileName)
}

@Test
Expand All @@ -227,9 +236,126 @@ internal class FileReaderTest {
testedReader.dropAllBatches()

// then
val sentBatches: MutableSet<String> = testedReader.getFieldValue("sentBatches")
val lockedFiles: MutableSet<String> = testedReader.getFieldValue("lockedFiles")
assertThat(rootDir.listFiles()).isEmpty()
assertThat(sentBatches).isEmpty()
assertThat(lockedFiles).isEmpty()
}

@Test
fun `it will do nothing if the only available file to be sent is locked`(forge: Forge) {
// given
val inProgressFileName = forge.anAlphabeticalString()
val inProgressFile = generateFile(inProgressFileName)
val countDownLatch = CountDownLatch(2)
whenever(mockOrchestrator.getReadableFile(emptySet()))
.thenReturn(inProgressFile)
.thenReturn(null)
whenever(mockOrchestrator.getReadableFile(setOf(inProgressFileName))).thenReturn(null)

var batch1: Batch? = null
var batch2: Batch? = null

// when
Thread {
batch1 = testedReader.readNextBatch()
Thread {
batch2 = testedReader.readNextBatch()
countDownLatch.countDown()
}.start()
countDownLatch.countDown()
}.start()

// then
countDownLatch.await(5, TimeUnit.SECONDS)
assertThat(batch1?.id).isEqualTo(inProgressFileName)
assertThat(batch2).isNull()
}

@Test
fun `it will return the next file if the current one is locked`(forge: Forge) {
// given
val inProgressFileName = forge.anAlphabeticalString()
val nextFileName = inProgressFileName + "_next"
val inProgressFile = generateFile(inProgressFileName)
val nextFile = generateFile(nextFileName)
val countDownLatch = CountDownLatch(2)
whenever(mockOrchestrator.getReadableFile(emptySet()))
.thenReturn(inProgressFile)
.thenReturn(null)
whenever(mockOrchestrator.getReadableFile(setOf(inProgressFileName))).thenReturn(nextFile)

var batch1: Batch? = null
var batch2: Batch? = null

// when
Thread {
batch1 = testedReader.readNextBatch()
Thread {
batch2 = testedReader.readNextBatch()
countDownLatch.countDown()
}.start()
countDownLatch.countDown()
}.start()

// then
countDownLatch.await(5, TimeUnit.SECONDS)
assertThat(batch1?.id).isEqualTo(inProgressFileName)
assertThat(batch2?.id).isEqualTo(nextFileName)
}

@Test
fun `it will return the released file`(forge: Forge) {
// given
val inProgressFileName = forge.anAlphabeticalString()
val nextFileName = inProgressFileName + "_next"
val inProgressFile = generateFile(inProgressFileName)
val nextFile = generateFile(nextFileName)
val countDownLatch = CountDownLatch(2)
whenever(mockOrchestrator.getReadableFile(emptySet()))
.thenReturn(inProgressFile)
whenever(mockOrchestrator.getReadableFile(setOf(inProgressFileName))).thenReturn(nextFile)

var batch2: Batch? = null

// when
Thread {
val batch1 = testedReader.readNextBatch()
Thread {
Thread.sleep(500) // give timet o first thread to release the batch
batch2 = testedReader.readNextBatch()
countDownLatch.countDown()
}.start()
batch1?.let {
testedReader.releaseBatch(it.id)
}
countDownLatch.countDown()
}.start()

// then
countDownLatch.await(5, TimeUnit.SECONDS)
assertThat(batch2?.id).isEqualTo(inProgressFileName)
}

@Test
fun `it will not throw exception in case of concurrent access`(forge: Forge) {
val file1 = generateFile(forge.anAlphabeticalString())
val file2 = generateFile(forge.anAlphabeticalString())
val file3 = generateFile(forge.anAlphabeticalString())
val file4 = generateFile(forge.anAlphabeticalString())
whenever(mockOrchestrator.getReadableFile(any()))
.thenReturn(file1)
.thenReturn(file2)
.thenReturn(file3)
.thenReturn(file4)
val countDownLatch = CountDownLatch(4)
repeat(4) {
Thread {
testedReader.readNextBatch()?.let { testedReader.releaseBatch(it.id) }
countDownLatch.countDown()
}.start()
}

countDownLatch.await(5, TimeUnit.SECONDS)
}

private fun generateFile(fileName: String): File {
Expand Down
20 changes: 14 additions & 6 deletions ...d/src/test/kotlin/com/datadog/android/core/internal/domain/FilePersistenceStrategyTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import fr.xgouchet.elmyr.Forge
import fr.xgouchet.elmyr.junit5.ForgeConfiguration
import fr.xgouchet.elmyr.junit5.ForgeExtension
import java.io.File
import java.util.concurrent.CountDownLatch
import java.util.concurrent.TimeUnit
import kotlin.math.min
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.AfterEach
Expand Down Expand Up @@ -268,21 +270,27 @@ internal abstract class FilePersistenceStrategyTest<T : Any>(
}

@Test
fun `read returns null when 1st batch is already sent but file still present`(
fun `reads null when batch already sent but the other thread is still trying to delete this`(
forge: Forge
) {
val fakeModel = forge.getForgery(modelClass)
testedWriter.write(fakeModel)
waitForNextBatch()
val countDownLatch = CountDownLatch(2)
val batch = testedReader.readNextBatch()
var batch2: Batch? = Batch("", ByteArray(0))
checkNotNull(batch)

testedReader.dropBatch(batch.id)
val logsDir = File(tempDir, dataFolderName)
val file = File(logsDir, batch.id)
file.writeText("I'm still there !")
val batch2 = testedReader.readNextBatch()
Thread {
testedReader.dropBatch(batch.id)
countDownLatch.countDown()
}.start()
Thread {
batch2 = testedReader.readNextBatch()
countDownLatch.countDown()
}.start()

countDownLatch.await(5, TimeUnit.SECONDS)
assertThat(batch2)
.isNull()
}
Expand Down

0 comments on commit 0f16d39

Please sign in to comment.