Lock local changes to assert transactions #203
Conversation
WalkthroughThe recent updates introduce a new test to ensure duplicated local changes aren't sent to the server, enhance concurrency management in document operations using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Document
participant Server
Client->>Document: Attach Document
Document->>Client: Document Attached
Client->>Document: Make Local Changes
Client->>Client: Check for Duplicates
alt No Duplicates
Client->>Server: Push Changes
else Duplicates Found
Client->>Server: No Action
end
Server->>Client: Acknowledge Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt (1 hunks)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (6 hunks)
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (3 hunks)
Additional comments not posted (2)
yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (1)
258-278: The refactoring of theapplyChangePackmethod appears to be well-structured and logically sound. However, ensure that the removal of Mutex does not introduce any race conditions, especially with the asynchronous operations involved.Verification successful
The
Document.ktfile useswithContextwith a dispatcher for various operations, ensuring thread safety. Thelaunchblock operates within a coroutine scope, which is consistent with the usage ofwithContextfor synchronization. Therefore, the removal of Mutex does not introduce race conditions.
- The
launchblock is within a coroutine scope, ensuring proper synchronization.- The surrounding code uses
withContextto ensure thread safety.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no race conditions are introduced due to Mutex removal. # Test: Search for concurrent modifications in Document class. Expect: Proper synchronization mechanisms in place. rg --type kotlin $'Document.*async|launch' yorkie/src/main/kotlin/dev/yorkie/document/Document.ktLength of output: 158
Script:
#!/bin/bash # Description: Locate and review the usage of `launch` in the Document.kt file to ensure proper synchronization mechanisms are in place. # Test: Search for the exact usage of `launch` in the Document.kt file. rg 'launch' yorkie/src/main/kotlin/dev/yorkie/document/Document.kt -A 10 -B 10Length of output: 1941
Script:
#!/bin/bash # Description: Check for synchronization mechanisms around the `launch` block in Document.kt. # Test: Search for usage of synchronization primitives or context ensuring thread safety. rg 'synchronized|Mutex|withContext' yorkie/src/main/kotlin/dev/yorkie/document/Document.kt -A 10 -B 10Length of output: 5980
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (1)
75-76: The introduction ofMutexfor document synchronization in theClientclass is a crucial enhancement for ensuring thread safety during document operations. The use ofwithLockin various methods likesyncAsync,attachAsync,detachAsync, andremoveAsyncensures that these operations are atomic and thread-safe. This is a significant improvement in handling concurrency issues.Also applies to: 114-118, 246-274
| @Test | ||
| fun test_duplicated_local_changes_not_sent_to_server() { | ||
| withTwoClientsAndDocuments(detachDocuments = false) { c1, c2, d1, d2, _ -> | ||
| val d1Events = mutableListOf<Document.Event>() | ||
| val d2Events = mutableListOf<Document.Event>() | ||
| val collectJobs = listOf( | ||
| launch(start = CoroutineStart.UNDISPATCHED) { | ||
| d1.events.filter { it is RemoteChange || it is LocalChange } | ||
| .collect(d1Events::add) | ||
| }, | ||
| launch(start = CoroutineStart.UNDISPATCHED) { | ||
| d2.events.filter { it is RemoteChange || it is LocalChange } | ||
| .collect(d2Events::add) | ||
| }, | ||
| ) | ||
|
|
||
| listOf( | ||
| d1.updateAsync { root, _ -> | ||
| root.setNewTree( | ||
| "t", | ||
| element("doc") { | ||
| element("p") { text { "12" } } | ||
| element("p") { text { "34" } } | ||
| }, | ||
| ) | ||
| }, | ||
| c1.syncAsync(), | ||
| c1.syncAsync(), | ||
| c1.syncAsync(), | ||
| c1.detachAsync(d1), | ||
| ) | ||
|
|
||
| withTimeout(GENERAL_TIMEOUT) { | ||
| while (d2Events.isEmpty()) { | ||
| delay(50) | ||
| } | ||
| } | ||
| assertIs<RemoteChange>(d2Events.first()) | ||
| assertTreesXmlEquals("<doc><p>12</p><p>34</p></doc>", d1, d2) | ||
|
|
||
| delay(500) | ||
|
|
||
| assert(d2Events.size == 1) | ||
|
|
||
| c2.detachAsync(d2).await() | ||
|
|
||
| collectJobs.forEach(Job::cancel) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test test_duplicated_local_changes_not_sent_to_server is well-structured and aligns with the PR's objective to prevent duplicated local changes from being sent to the server. However, consider adding more detailed comments within the test to explain each step, especially the multiple syncAsync calls and their purpose in this context. This will improve maintainability and readability for other developers who may work on this test in the future.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
What this PR does / why we need it?
When detach and other changes happen almost asynchonously, duplicated local changes could be sent to the server.
This PR uses mutex to assert transactions for push-pull and local application.
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores