Skip to content

Commit

Permalink
Pass better information of operation failures (#179)
Browse files Browse the repository at this point in the history
* return Result rather than booleans for public operations
  • Loading branch information
skhugh authored May 14, 2024
1 parent 5631a8e commit 2ac770b
Show file tree
Hide file tree
Showing 9 changed files with 78 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ class KanbanBoardViewModel(private val client: Client) : ViewModel() {

init {
viewModelScope.launch {
if (client.activateAsync().await() && client.attachAsync(document).await()) {
if (client.activateAsync().await().isSuccess &&
client.attachAsync(document).await().isSuccess
) {
client.syncAsync().await()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ class EditorViewModel(private val client: Client) : ViewModel(), YorkieEditText.
}.await()
}

if (client.activateAsync().await() && client.attachAsync(document).await()) {
if (client.activateAsync().await().isSuccess &&
client.attachAsync(document).await().isSuccess
) {
client.syncAsync().await()
}
}
Expand Down
16 changes: 8 additions & 8 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@
minSdk = "24"
compileSdk = "34"
targetSdk = "34"
agp = "8.3.2"
connectKotlin = "0.6.0"
agp = "8.4.0"
connectKotlin = "0.6.1"
okhttp = "4.12.0"
coroutines = "1.8.0"
coroutines = "1.8.1"
androidxActivity = "1.9.0"
androidxLifecycle = "2.7.0"
androidxBenchmark = "1.2.3"
androidxComposeCompiler = "1.5.11"
androidxBenchmark = "1.2.4"
androidxComposeCompiler = "1.5.13"

[libraries]
androidx-annotation = { module = "androidx.annotation:annotation", version = "1.7.1" }
Expand All @@ -24,7 +24,7 @@ kotlinx-collections-immutable = { group = "org.jetbrains.kotlinx", name = "kotli

apache-commons-collections = { group = "org.apache.commons", name = "commons-collections4", version = "4.4" }

androidx-core = { group = "androidx.core", name = "core-ktx", version = "1.13.0" }
androidx-core = { group = "androidx.core", name = "core-ktx", version = "1.13.1" }
androidx-appcompat = { group = "androidx.appcompat", name = "appcompat", version = "1.6.1" }
androidx-activity = { group = "androidx.activity", name = "activity-ktx", version.ref = "androidxActivity" }
androidx-activity-compose = { group = "androidx.activity", name = "activity-compose", version.ref = "androidxActivity" }
Expand All @@ -33,14 +33,14 @@ androidx-lifecycle-viewmodel = { group = "androidx.lifecycle", name = "lifecycle
androidx-lifecycle-runtime = { group = "androidx.lifecycle", name = "lifecycle-runtime-ktx", version.ref = "androidxLifecycle" }
androidx-benchmark = { group = "androidx.benchmark", name = "benchmark-junit4", version.ref = "androidxBenchmark" }

androidx-compose-bom = { group = "androidx.compose", name = "compose-bom", version = "2024.04.01" }
androidx-compose-bom = { group = "androidx.compose", name = "compose-bom", version = "2024.05.00" }
androidx-compose-ui = { module = "androidx.compose.ui:ui" }
androidx-compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling" }
androidx-compose-ui-tooling-preview = { module = "androidx.compose.ui:ui-tooling-preview" }
androidx-compose-ui-test-manifest = { module = "androidx.compose.ui:ui-test-manifest" }
androidx-compose-material = { module = "androidx.compose.material:material" }

material = { group = "com.google.android.material", name = "material", version = "1.11.0" }
material = { group = "com.google.android.material", name = "material", version = "1.12.0" }

gson = { group = "com.google.code.gson", name = "gson", version = "2.10.1" }

Expand Down
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#Fri Jun 16 11:23:01 KST 2023
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-8.6-bin.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
63 changes: 34 additions & 29 deletions yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import dev.yorkie.document.Document
import dev.yorkie.document.Document.DocumentStatus
import dev.yorkie.document.Document.Event.PresenceChange
import dev.yorkie.document.time.ActorID
import dev.yorkie.util.OperationResult
import dev.yorkie.util.SUCCESS
import dev.yorkie.util.YorkieLogger
import dev.yorkie.util.createSingleThreadDispatcher
import java.io.Closeable
Expand Down Expand Up @@ -148,24 +150,24 @@ public class Client @VisibleForTesting internal constructor(
* and receives a unique ID from the server. The given ID is used to
* distinguish different clients.
*/
public fun activateAsync(): Deferred<Boolean> {
public fun activateAsync(): Deferred<OperationResult> {
return scope.async {
if (isActive) {
return@async true
return@async SUCCESS
}
val activateResponse = service.activateClient(
activateClientRequest {
clientKey = options.key
},
projectBasedRequestHeader,
).getOrElse {
YorkieLogger.e("Client.activate", it.toString())
return@async false
ensureActive()
return@async Result.failure(it)
}
_status.emit(Status.Activated(ActorID(activateResponse.clientId)))
runSyncLoop()
runWatchLoop()
true
SUCCESS
}
}

Expand Down Expand Up @@ -198,9 +200,9 @@ public class Client @VisibleForTesting internal constructor(
* Pushes local changes of the attached documents to the server and
* receives changes of the remote replica from the server then apply them to local documents.
*/
public fun syncAsync(document: Document? = null): Deferred<Boolean> {
public fun syncAsync(document: Document? = null): Deferred<OperationResult> {
return scope.async {
var isAllSuccess = true
var failure: Throwable? = null
val attachments = document?.let {
listOf(
attachments.value[it.key]?.copy(syncMode = SyncMode.Realtime)
Expand All @@ -212,12 +214,15 @@ public class Client @VisibleForTesting internal constructor(
if (result.isSuccess) {
DocumentSynced(Synced(document))
} else {
isAllSuccess = false
DocumentSynced(SyncFailed(document, result.exceptionOrNull()))
val exception = result.exceptionOrNull()
if (failure == null && exception != null) {
failure = exception
}
DocumentSynced(SyncFailed(document, exception))
},
)
}
isAllSuccess
failure?.let { Result.failure(it) } ?: SUCCESS
}
}

Expand Down Expand Up @@ -452,7 +457,7 @@ public class Client @VisibleForTesting internal constructor(
document: Document,
initialPresence: P = emptyMap(),
syncMode: SyncMode = SyncMode.Realtime,
): Deferred<Boolean> {
): Deferred<OperationResult> {
return scope.async {
check(isActive) {
"client is not active"
Expand All @@ -473,14 +478,14 @@ public class Client @VisibleForTesting internal constructor(
request,
document.key.documentBasedRequestHeader,
).getOrElse {
YorkieLogger.e("Client.attach", it.toString())
return@async false
ensureActive()
return@async Result.failure(it)
}
val pack = response.changePack.toChangePack()
document.applyChangePack(pack)

if (document.status == DocumentStatus.Removed) {
return@async true
return@async SUCCESS
}

document.status = DocumentStatus.Attached
Expand All @@ -490,7 +495,7 @@ public class Client @VisibleForTesting internal constructor(
syncMode,
)
waitForInitialization(document.key)
true
SUCCESS
}
}

Expand All @@ -502,7 +507,7 @@ public class Client @VisibleForTesting internal constructor(
* the changes should be applied to other replicas before GC time. For this,
* if the [document] is no longer used by this [Client], it should be detached.
*/
public fun detachAsync(document: Document): Deferred<Boolean> {
public fun detachAsync(document: Document): Deferred<OperationResult> {
return scope.async {
check(isActive) {
"client is not active"
Expand All @@ -523,26 +528,26 @@ public class Client @VisibleForTesting internal constructor(
request,
document.key.documentBasedRequestHeader,
).getOrElse {
YorkieLogger.e("Client.detach", it.toString())
return@async false
ensureActive()
return@async Result.failure(it)
}
val pack = response.changePack.toChangePack()
document.applyChangePack(pack)
if (document.status != DocumentStatus.Removed) {
document.status = DocumentStatus.Detached
attachments.value -= document.key
}
true
SUCCESS
}
}

/**
* Deactivates this [Client].
*/
public fun deactivateAsync(): Deferred<Boolean> {
public fun deactivateAsync(): Deferred<OperationResult> {
return scope.async {
if (!isActive) {
return@async true
return@async SUCCESS
}
activationJob.cancelChildren()
_streamConnectionStatus.emit(StreamConnectionStatus.Disconnected)
Expand All @@ -553,18 +558,18 @@ public class Client @VisibleForTesting internal constructor(
},
projectBasedRequestHeader,
).getOrElse {
YorkieLogger.e("Client.deactivate", it.toString())
return@async false
ensureActive()
return@async Result.failure(it)
}
_status.emit(Status.Deactivated)
true
SUCCESS
}
}

/**
* Removes the given [document].
*/
public fun removeAsync(document: Document): Deferred<Boolean> {
public fun removeAsync(document: Document): Deferred<OperationResult> {
return scope.async {
check(isActive) {
"client is not active"
Expand All @@ -581,13 +586,13 @@ public class Client @VisibleForTesting internal constructor(
request,
document.key.documentBasedRequestHeader,
).getOrElse {
YorkieLogger.e("Client.remove", it.toString())
return@async false
ensureActive()
return@async Result.failure(it)
}
val pack = response.changePack.toChangePack()
document.applyChangePack(pack)
attachments.value -= document.key
true
SUCCESS
}
}

Expand Down Expand Up @@ -623,7 +628,7 @@ public class Client @VisibleForTesting internal constructor(
streamClient.dispatcher.executorService.shutdown()
}

private data class SyncResult(val document: Document, val result: Result<Unit>)
private data class SyncResult(val document: Document, val result: OperationResult)

private class WatchJobHolder(val documentID: String, val job: Job)

Expand Down
18 changes: 11 additions & 7 deletions yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import dev.yorkie.document.operation.OperationInfo
import dev.yorkie.document.time.ActorID
import dev.yorkie.document.time.TimeTicket
import dev.yorkie.document.time.TimeTicket.Companion.InitialTimeTicket
import dev.yorkie.util.OperationResult
import dev.yorkie.util.YorkieLogger
import dev.yorkie.util.createSingleThreadDispatcher
import java.io.Closeable
Expand All @@ -36,6 +37,7 @@ import kotlinx.coroutines.Deferred
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.async
import kotlinx.coroutines.cancel
import kotlinx.coroutines.ensureActive
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.MutableStateFlow
Expand Down Expand Up @@ -129,7 +131,7 @@ public class Document(
public fun updateAsync(
message: String? = null,
updater: suspend (root: JsonObject, presence: Presence) -> Unit,
): Deferred<Boolean> {
): Deferred<OperationResult> {
return scope.async {
check(status != DocumentStatus.Removed) {
"document is removed"
Expand All @@ -143,20 +145,22 @@ public class Document(
)
val actorID = changeID.actor

runCatching {
val result = runCatching {
val proxy = JsonObject(context, clone.root.rootObject)
updater.invoke(
proxy,
Presence(context, clone.presences[changeID.actor].orEmpty()),
)
}.onFailure {
this@Document.clone = null
YorkieLogger.e("Document.update", it.message.orEmpty())
return@async false
ensureActive()
}
if (result.isFailure) {
return@async result
}

if (!context.hasChange) {
return@async true
return@async result
}
val change = context.getChange()
val (operationInfos, newPresences) = change.execute(root, _presences.value)
Expand All @@ -169,12 +173,12 @@ public class Document(
}
if (change.hasPresenceChange) {
val presence =
newPresences?.get(actorID) ?: _presences.value[actorID] ?: return@async false
newPresences?.get(actorID) ?: _presences.value[actorID] ?: return@async result
newPresences?.let {
emitPresences(it, createPresenceChangedEvent(actorID, presence))
}
}
true
result
}
}

Expand Down
5 changes: 5 additions & 0 deletions yorkie/src/main/kotlin/dev/yorkie/util/Result.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package dev.yorkie.util

typealias OperationResult = Result<Unit>

val SUCCESS = Result.success(Unit)
Loading

0 comments on commit 2ac770b

Please sign in to comment.