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

fix: resolve deadlocks by narrowing the scope of locks #2778

Conversation

mechairoi
Copy link
Contributor

@mechairoi mechairoi commented May 9, 2024

Summary

Resolve deadlocks by narrowing the scope of locks.
This change could be related to Issue #306, but its relevance is not guaranteed.

syntax -> documents

match self.syntax.entry(biome_path) {
Entry::Occupied(entry) => Ok(entry.get().clone()),
Entry::Vacant(entry) => {
let biome_path = entry.key();
let capabilities = self.get_file_capabilities(biome_path);
let mut document = self
.documents

documents -> syntax (will change)

let mut document = self
.documents
.get_mut(&params.path)
.ok_or_else(WorkspaceError::not_found)?;
debug_assert!(params.version > document.version);
document.version = params.version;
document.content = params.content;
self.syntax.remove(&params.path);

Test Plan

while true; do; (prepare many not formatted files) && RUST_BACKTRACE=1 biome format --write; done;

@github-actions github-actions bot added A-Project Area: project A-Changelog Area: changelog labels May 9, 2024
@ematipico
Copy link
Member

Thank you for looking after this. When dealing with deadlocks, what I found useful and guaranteed, is the use of small functions that do a single operation over a lock, so we take advantage of the fact that the lock is released when the scope of the function terminates.

In this case, other than moving the order of the function, maybe we should create two new functions that add and remove a path to syntax. What do you think?

@mechairoi
Copy link
Contributor Author

I agree that creating functions is a good idea. Additionally, your suggestion inspired me to think that introducing nested scopes could be beneficial as well. The following changes also worked on my machine.

--- a/crates/biome_service/src/workspace/server.rs
+++ b/crates/biome_service/src/workspace/server.rs
@@ -548,14 +548,16 @@ impl Workspace for WorkspaceServer {

     /// Change the content of an open file
     fn change_file(&self, params: ChangeFileParams) -> Result<(), WorkspaceError> {
-        let mut document = self
-            .documents
-            .get_mut(&params.path)
-            .ok_or_else(WorkspaceError::not_found)?;
+        {
+            let mut document = self
+                .documents
+                .get_mut(&params.path)
+                .ok_or_else(WorkspaceError::not_found)?;

-        debug_assert!(params.version > document.version);
-        document.version = params.version;
-        document.content = params.content;
+            debug_assert!(params.version > document.version);
+            document.version = params.version;
+            document.content = params.content;
+        }

         self.syntax.remove(&params.path);
         Ok(())

@mechairoi mechairoi force-pushed the fix-resolve-deadlocks-by-acquiring-locks-in-the-smae-order branch from 5bdc500 to 96691a1 Compare May 9, 2024 11:27
@mechairoi mechairoi changed the title fix: resolve deadlocks by acquiring locks in the same order fix: resolve deadlocks by narrowing the scope of locks May 9, 2024
@ematipico ematipico merged commit 780080f into biomejs:main May 10, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants