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.

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

feat: refactor file storage to fix thread safety issues #144

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import com.amplitude.android.utilities.AndroidStorage
import com.amplitude.common.Logger
import com.amplitude.core.Storage
import java.io.File
import java.util.UUID

class StorageKeyMigration(
private val source: AndroidStorage,
Expand All @@ -29,22 +28,22 @@ class StorageKeyMigration(

for (sourceEventFilePath in sourceEventFiles) {
val sourceEventFile = File(sourceEventFilePath)
var destinationEventFile =
File(sourceEventFilePath.replace(source.storageKey, destination.storageKey))
val destinationFilePath = sourceEventFilePath.replace(
"/${source.storageKey}/",
"/${destination.storageKey}/"
).replace(
source.eventsFile.id,
destination.eventsFile.id,
)
val destinationEventFile = File(destinationFilePath)
if (destinationEventFile.exists()) {
var fileExtension = destinationEventFile.extension
if (fileExtension != "") {
fileExtension = ".$fileExtension"
logger.error("destination ${destinationEventFile.absoluteFile} already exists")
justin-fiedler marked this conversation as resolved.
Show resolved Hide resolved
} else {
try {
sourceEventFile.renameTo(destinationEventFile)
} catch (e: Exception) {
logger.error("can't rename $sourceEventFile to $destinationEventFile: ${e.message}")
}
destinationEventFile = File(
destinationEventFile.parent,
"${destinationEventFile.nameWithoutExtension}-${UUID.randomUUID()}$fileExtension"
)
}
try {
sourceEventFile.renameTo(destinationEventFile)
} catch (e: Exception) {
logger.error("can't rename $sourceEventFile to $destinationEventFile: ${e.message}")
}
}
} catch (e: Exception) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import com.amplitude.core.platform.EventPipeline
import com.amplitude.core.utilities.EventsFileManager
import com.amplitude.core.utilities.EventsFileStorage
import com.amplitude.core.utilities.FileResponseHandler
import com.amplitude.core.utilities.JSONUtil
import com.amplitude.core.utilities.ResponseHandler
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.CoroutineScope
Expand All @@ -34,12 +33,12 @@ class AndroidStorage(
internal val sharedPreferences: SharedPreferences =
context.getSharedPreferences("${getPrefix()}-$storageKey", Context.MODE_PRIVATE)
private val storageDirectory: File = context.getDir(getDir(), Context.MODE_PRIVATE)
private val eventsFile =
EventsFileManager(storageDirectory, storageKey, AndroidKVS(sharedPreferences))
internal val eventsFile =
EventsFileManager(storageDirectory, storageKey, logger)
private val eventCallbacksMap = mutableMapOf<String, EventCallBack>()

override suspend fun writeEvent(event: BaseEvent) {
eventsFile.storeEvent(JSONUtil.eventToString(event))
eventsFile.storeEvent(event)
event.callback?.let { callback ->
event.insertId?.let {
eventCallbacksMap.put(it, callback)
Expand Down Expand Up @@ -67,11 +66,7 @@ class AndroidStorage(
return eventsFile.read()
}

override fun releaseFile(filePath: String) {
eventsFile.release(filePath)
}

override suspend fun getEventsString(content: Any): String {
override fun getEventsString(content: Any): String {
return eventsFile.getEventString(content as String)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,50 +145,6 @@ class StorageKeyMigrationTest {
Assertions.assertEquals(0, destinationEventFiles.size)
}

@Test
fun `event files with duplicated names should be migrated`() {
val context = ApplicationProvider.getApplicationContext<Context>()
val logger = ConsoleLogger()

val source = AndroidStorage(context, UUID.randomUUID().toString(), logger, null)
val destination = AndroidStorage(context, UUID.randomUUID().toString(), logger, null)

runBlocking {
source.writeEvent(createEvent(1))
source.rollover()
source.writeEvent(createEvent(22))
source.rollover()
}

val sourceEventFiles = source.readEventsContent() as List<String>
Assertions.assertEquals(2, sourceEventFiles.size)

val sourceFileSizes = sourceEventFiles.map { File(it).length() }

runBlocking {
destination.writeEvent(createEvent(333))
destination.rollover()
}

var destinationEventFiles = destination.readEventsContent() as List<String>
Assertions.assertEquals(1, destinationEventFiles.size)

val destinationFileSizes = destinationEventFiles.map { File(it).length() }

val migration = StorageKeyMigration(source, destination, logger)
runBlocking {
migration.execute()
}

destinationEventFiles = destination.readEventsContent() as List<String>
Assertions.assertEquals("-0", destinationEventFiles[0].substring(destinationEventFiles[0].length - 2))
Assertions.assertTrue(destinationEventFiles[1].contains("-0-"))
Assertions.assertEquals("-1", destinationEventFiles[2].substring(destinationEventFiles[0].length - 2))
Assertions.assertEquals(destinationFileSizes[0], File(destinationEventFiles[0]).length())
Assertions.assertEquals(sourceFileSizes[0], File(destinationEventFiles[1]).length())
Assertions.assertEquals(sourceFileSizes[1], File(destinationEventFiles[2]).length())
}

private fun createEvent(eventIndex: Int): BaseEvent {
val event = BaseEvent()
event.eventType = "event-$eventIndex"
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/amplitude/core/Storage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ interface Storage {

fun readEventsContent(): List<Any>

suspend fun getEventsString(content: Any): String
fun getEventsString(content: Any): String

fun getResponseHandler(eventPipeline: EventPipeline, configuration: Configuration, scope: CoroutineScope, dispatcher: CoroutineDispatcher): ResponseHandler
}
Expand Down
Loading