From c929f363338ee85d951c1cea930629d6577768a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E1=84=80=E1=85=B5=E1=86=B7=E1=84=92=E1=85=AD=E1=84=8B?= =?UTF-8?q?=E1=85=AE=5BSE=5D=5BSmartEditor=5D?= Date: Mon, 3 Jun 2024 09:55:09 +0900 Subject: [PATCH 1/2] Tests for concurrent tree modifications causing incorrect remote event paths --- .../dev/yorkie/document/json/JsonTreeTest.kt | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt index f589f3ecb..0b056b0b8 100644 --- a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -37,6 +37,7 @@ import kotlinx.coroutines.flow.filterIsInstance import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.joinAll import kotlinx.coroutines.launch import kotlinx.coroutines.withTimeout import org.junit.Test @@ -2159,6 +2160,78 @@ class JsonTreeTest { } } + // Tests for concurrent tree modifications causing incorrect remote event paths. + @Test + fun test_concurrent_tree_style_and_insertion() { + withTwoClientsAndDocuments(syncMode = Client.SyncMode.Manual) { c1, c2, d1, d2, _ -> + updateAndSync( + Updater(c1, d1) { root, _ -> + root.setNewTree( + "t", + element("doc") { + element("p") + }, + ) + }, + Updater(c2, d2), + ) + assertTreesXmlEquals("""

""", d1, d2) + + val d1Events = mutableListOf() + val d2Events = mutableListOf() + + val collectJob = launch(start = CoroutineStart.UNDISPATCHED) { + launch(start = CoroutineStart.UNDISPATCHED) { + d1.events.filterIsInstance() + .map { + it.changeInfo.operations.filterIsInstance() + } + .collect(d1Events::addAll) + } + launch(start = CoroutineStart.UNDISPATCHED) { + d2.events.filterIsInstance() + .map { + it.changeInfo.operations.filterIsInstance() + } + .collect(d2Events::addAll) + } + } + + listOf( + launch { + repeat(10) { + d1.updateAsync { root, _ -> + root.rootTree().style(0, 1, mapOf("style" to it.toString())) + }.await() + } + }, + launch { + d2.updateAsync { root, _ -> + root.rootTree().edit(0, 0, element("p2")) + }.await() + }, + ).joinAll() + + c1.syncAsync().await() + c2.syncAsync().await() + c1.syncAsync().await() + + // the resulting trees are the same. + assertEquals(d1.getRoot().rootTree().toXml(), d2.getRoot().rootTree().toXml()) + + // insert event is sent with path [0] + assertEquals(listOf(0), d1Events.single().fromPath) + + // but here test fails. + // style event should be sent with path [1], but path [0] is given. + d2Events.forEach { + assertEquals(listOf(1), it.fromPath) + } + + collectJob.cancel() + } + } + companion object { fun JsonObject.rootTree() = getAs("t") From a9cd71636773c4e1389ea5d5069496f0437623a1 Mon Sep 17 00:00:00 2001 From: jee-hyun-kim Date: Wed, 5 Jun 2024 16:32:23 +0900 Subject: [PATCH 2/2] Fix incorrect indexes in TreeChange --- docker/docker-compose-ci.yml | 2 +- docker/docker-compose.yml | 2 +- .../document/json/JsonTreeConcurrencyTest.kt | 6 +- .../dev/yorkie/document/json/JsonTreeTest.kt | 74 ++++++++++++++++++- .../dev/yorkie/document/crdt/CrdtTree.kt | 53 ++++++++----- .../kotlin/dev/yorkie/document/crdt/Rht.kt | 1 + .../dev/yorkie/document/crdt/TreeInfo.kt | 2 +- .../dev/yorkie/document/json/JsonTree.kt | 8 +- .../document/operation/OperationInfo.kt | 1 + .../document/operation/TreeStyleOperation.kt | 3 + .../main/kotlin/dev/yorkie/util/IndexTree.kt | 6 ++ .../dev/yorkie/document/json/JsonTreeTest.kt | 2 + 12 files changed, 133 insertions(+), 27 deletions(-) diff --git a/docker/docker-compose-ci.yml b/docker/docker-compose-ci.yml index 927d16bf9..0ed3e87cc 100644 --- a/docker/docker-compose-ci.yml +++ b/docker/docker-compose-ci.yml @@ -2,7 +2,7 @@ version: '3.3' services: yorkie: - image: 'yorkieteam/yorkie:0.4.19' + image: 'yorkieteam/yorkie:0.4.22' container_name: 'yorkie' command: ['server', '--mongo-connection-uri', 'mongodb://mongo:27017'] restart: always diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 62c1ec5e8..98a1fc1e0 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -2,7 +2,7 @@ version: '3.3' services: yorkie: - image: 'yorkieteam/yorkie:0.4.20' + image: 'yorkieteam/yorkie:0.4.22' container_name: 'yorkie' command: ['server', '--enable-pprof'] restart: always diff --git a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt index 595720002..0c0f29c8c 100644 --- a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt +++ b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt @@ -313,10 +313,10 @@ class JsonTreeConcurrencyTest { } } val initialXml = - "

a

b

c

" + """

a

b

c

""" val content = element("p") { text { "d" } - attrs { mapOf("italic" to "true") } + attrs { mapOf("italic" to "true", "color" to "blue") } } val ranges = listOf( @@ -387,7 +387,7 @@ class JsonTreeConcurrencyTest { StyleOpCode.StyleRemove, "color", "", - "remove-bold", + "remove-color", ), StyleOperationType( RangeSelector.RangeAll, diff --git a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt index 0b056b0b8..32cd62e1d 100644 --- a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -154,7 +154,7 @@ class JsonTreeTest { Updater(c2, d2), ) assertTreesXmlEquals( - """

hello

""", + """

hello

""", d1, d2, ) @@ -2222,8 +2222,76 @@ class JsonTreeTest { // insert event is sent with path [0] assertEquals(listOf(0), d1Events.single().fromPath) - // but here test fails. - // style event should be sent with path [1], but path [0] is given. + d2Events.forEach { + assertEquals(listOf(1), it.fromPath) + } + + collectJob.cancel() + } + } + + @Test + fun test_concurrent_tree_remove_style_and_insertion() { + withTwoClientsAndDocuments(syncMode = Client.SyncMode.Manual) { c1, c2, d1, d2, _ -> + updateAndSync( + Updater(c1, d1) { root, _ -> + root.setNewTree( + "t", + element("doc") { + element("p") { + attrs { mapOf("key" to "a") } + } + }, + ) + }, + Updater(c2, d2), + ) + assertTreesXmlEquals("""

""", d1, d2) + + val d1Events = mutableListOf() + val d2Events = mutableListOf() + + val collectJob = launch(start = CoroutineStart.UNDISPATCHED) { + launch(start = CoroutineStart.UNDISPATCHED) { + d1.events.filterIsInstance() + .map { + it.changeInfo.operations.filterIsInstance() + } + .collect(d1Events::addAll) + } + launch(start = CoroutineStart.UNDISPATCHED) { + d2.events.filterIsInstance() + .map { + it.changeInfo.operations.filterIsInstance() + } + .collect(d2Events::addAll) + } + } + + listOf( + launch { + repeat(10) { + d1.updateAsync { root, _ -> + root.rootTree().removeStyle(0, 1, listOf("key")) + }.await() + } + }, + launch { + d2.updateAsync { root, _ -> + root.rootTree().edit(0, 0, element("p2")) + }.await() + }, + ).joinAll() + + c1.syncAsync().await() + c2.syncAsync().await() + c1.syncAsync().await() + + // the resulting trees are the same. + assertEquals(d1.getRoot().rootTree().toXml(), d2.getRoot().rootTree().toXml()) + + // insert event is sent with path [0] + assertEquals(listOf(0), d1Events.single().fromPath) d2Events.forEach { assertEquals(listOf(1), it.fromPath) } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt index 9dfcac068..cab39a705 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt @@ -23,6 +23,8 @@ public typealias TreePosRange = Pair internal typealias CrdtTreeToken = TreeToken +internal typealias TreeNodePair = Pair + internal data class CrdtTree( val root: CrdtTreeNode, override val createdAt: TimeTicket, @@ -93,6 +95,9 @@ internal data class CrdtTree( createdAtMapByActor[actorID] = createdAt } + val parentOfNode = requireNotNull(node.parent) + val previousNode = node.prevSibling ?: parentOfNode + val updatedAttrPairs = node.setAttributes(attributes, executedAt) val affectedAttrs = updatedAttrPairs.fold(emptyMap()) { acc, pair -> val curr = pair.new @@ -101,10 +106,10 @@ internal data class CrdtTree( if (affectedAttrs.isNotEmpty()) { TreeChange( type = TreeChangeType.Style, - from = toIndex(fromParent, fromLeft), - to = toIndex(toParent, toLeft), - fromPath = toPath(fromParent, fromLeft), - toPath = toPath(toParent, toLeft), + from = toIndex(parentOfNode, previousNode), + to = toIndex(node, node), + fromPath = toPath(parentOfNode, previousNode), + toPath = toPath(node, node), actorID = executedAt.actorID, attributes = attributes.filterKeys { it in affectedAttrs }, ).let(changes::add) @@ -423,31 +428,48 @@ internal data class CrdtTree( range: TreePosRange, attributeToRemove: List, executedAt: TimeTicket, + maxCreatedAtMapByActor: Map? = null, ): TreeOperationResult { val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt) val (toParent, toLeft) = findNodesAndSplitText(range.second, executedAt) val changes = mutableListOf() + val createdAtMapByActor = mutableMapOf() val gcPairs = mutableListOf>() traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { (node, _), _ -> - if (!node.isRemoved && !node.isText && attributeToRemove.isNotEmpty()) { + val actorID = node.createdAt.actorID + val maxCreatedAt = maxCreatedAtMapByActor?.let { + maxCreatedAtMapByActor[actorID] ?: InitialTimeTicket + } ?: MaxTimeTicket + + if (node.canStyle(executedAt, maxCreatedAt) && attributeToRemove.isNotEmpty()) { + val max = createdAtMapByActor[actorID] + val createdAt = node.createdAt + if (max < createdAt) { + createdAtMapByActor[actorID] = createdAt + } + attributeToRemove.forEach { key -> node.removeAttribute(key, executedAt) .map { rhtNode -> GCPair(node, rhtNode) } .let(gcPairs::addAll) } + + val parentOfNode = requireNotNull(node.parent) + val previousNode = node.prevSibling ?: parentOfNode + TreeChange( type = TreeChangeType.RemoveStyle, - from = toIndex(fromParent, fromLeft), - to = toIndex(toParent, toLeft), - fromPath = toPath(fromParent, fromLeft), - toPath = toPath(toParent, toLeft), + from = toIndex(parentOfNode, previousNode), + to = toIndex(node, node), + fromPath = toPath(parentOfNode, previousNode), + toPath = toPath(node, node), actorID = executedAt.actorID, attributesToRemove = attributeToRemove, ).let(changes::add) } } - return TreeOperationResult(changes, gcPairs) + return TreeOperationResult(changes, gcPairs, createdAtMapByActor) } private fun traverseInPosRange( @@ -475,12 +497,9 @@ internal data class CrdtTree( * If [executedAt] is given, then it is used to find the appropriate left node * for concurrent insertion. */ - fun findNodesAndSplitText( - pos: CrdtTreePos, - executedAt: TimeTicket? = null, - ): Pair { + fun findNodesAndSplitText(pos: CrdtTreePos, executedAt: TimeTicket? = null): TreeNodePair { // 01. Find the parent and left sibling node of the given position. - val (parent, leftSibling) = pos.toTreeNodes(this) + val (parent, leftSibling) = pos.toTreeNodePair(this) // 02. Determine whether the position is left-most and the exact parent // in the current tree. @@ -661,7 +680,7 @@ internal data class CrdtTree( /** * Converts the pos to parent and left sibling nodes. */ - private fun CrdtTreePos.toTreeNodes(tree: CrdtTree): Pair { + private fun CrdtTreePos.toTreeNodePair(tree: CrdtTree): TreeNodePair { val parentNode = tree.findFloorNode(parentID) val leftNode = tree.findFloorNode(leftSiblingID) require(parentNode != null && leftNode != null) { @@ -888,7 +907,7 @@ internal data class CrdtTreeNode private constructor( if (isText) { return false } - return createdAt <= maxCreatedAt && (removedAt == null || removedAt < executedAt) + return createdAt <= maxCreatedAt && removedAt < executedAt } override fun delete(node: RhtNode) { diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt index 5ce5720a4..5ac430552 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt @@ -96,6 +96,7 @@ internal class Rht : Collection { */ fun toXml(): String { return nodeMapByKey.filterValues { !it.isRemoved }.entries + .sortedBy { it.key } .joinToString(" ") { (key, node) -> "$key=\"${node.value}\"" } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt index ed825b3f1..440cf687c 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt @@ -81,5 +81,5 @@ internal enum class TreeChangeType { internal data class TreeOperationResult( val changes: List, val gcPairs: List> = emptyList(), - val maxCreatedAtMap: Map = emptyMap(), + val maxCreatedAtMap: Map, ) diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt index 091daf074..116b9873b 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt @@ -92,7 +92,12 @@ public class JsonTree internal constructor( val fromPos = target.findPos(fromIndex) val toPos = target.findPos(toIndex) val executedAt = context.issueTimeTicket() - target.removeStyle(fromPos to toPos, attributesToRemove, executedAt) + val (_, gcPairs, maxCreatedAtMapByActor) = target.removeStyle( + fromPos to toPos, + attributesToRemove, + executedAt, + ) + gcPairs.forEach(context::registerGCPair) context.push( TreeStyleOperation( @@ -100,6 +105,7 @@ public class JsonTree internal constructor( fromPos, toPos, executedAt, + maxCreatedAtMapByActor, attributesToRemove = attributesToRemove, ), ) diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt b/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt index 42e50cc02..9eaa9b26b 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt @@ -92,6 +92,7 @@ public sealed class OperationInfo { val from: Int, val to: Int, val fromPath: List, + val toPath: List, val attributes: Map = emptyMap(), val attributesToRemove: List = emptyList(), override var path: String = INITIAL_PATH, diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt b/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt index 3b802b1f9..6098c7ace 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt @@ -47,6 +47,7 @@ internal data class TreeStyleOperation( it.from, it.to, it.fromPath, + it.toPath, it.attributes.orEmpty(), path = root.createPath(parentCreatedAt), ) @@ -58,6 +59,7 @@ internal data class TreeStyleOperation( fromPos to toPos, attributesToRemove, executedAt, + maxCreatedAtMapByActor, ) gcPairs.forEach(root::registerGCPair) changes.map { @@ -65,6 +67,7 @@ internal data class TreeStyleOperation( it.from, it.to, it.fromPath, + it.toPath, attributesToRemove = it.attributesToRemove.orEmpty(), path = root.createPath(parentCreatedAt), ) diff --git a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt index 176e691b0..95b509d85 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt @@ -405,6 +405,12 @@ internal abstract class IndexTreeNode> { return parent?.children?.getOrNull(offset + 1) } + val prevSibling: T? + get() { + val offset = parent?.findOffset(this as T) ?: return null + return parent?.children?.getOrNull(offset - 1) + } + /** * Returns the children of the node. * Tombstone nodes remain awhile in the tree during editing. diff --git a/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt index aa63be930..0ce66e248 100644 --- a/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -320,6 +320,7 @@ class JsonTreeTest { 0, 1, listOf(0), + listOf(0, 0), mapOf("a" to "b"), path = "$.t", ), @@ -386,6 +387,7 @@ class JsonTreeTest { 2, 3, listOf(0, 0, 0), + listOf(0, 0, 0, 0), mapOf("a" to "b"), path = "$.t", ),