Don't allow multiple concurrent formatting requests for the same buffer

Max Brunsfeld and Nathan Sobo created 3 years ago

Co-authored-by: Nathan Sobo <[e-mail]>

Change summary

crates/editor/src/editor_tests.rs | 57 +++++++++++++++++++++++++++++++++
crates/project/src/project.rs     | 33 +++++++++++++++----
2 files changed, 83 insertions(+), 7 deletions(-)

Detailed changes

crates/editor/src/editor_tests.rs

@@ -3845,6 +3845,63 @@ async fn test_document_format_manual_trigger(cx: &mut gpui::TestAppContext) {
     );
 }
 
+#[gpui::test]
+async fn test_concurrent_format_requests(cx: &mut gpui::TestAppContext) {
+    cx.foreground().forbid_parking();
+
+    let mut cx = EditorLspTestContext::new_rust(
+        lsp::ServerCapabilities {
+            document_formatting_provider: Some(lsp::OneOf::Left(true)),
+            ..Default::default()
+        },
+        cx,
+    )
+    .await;
+
+    cx.set_state(indoc! {"
+        one.twoˇ
+    "});
+
+    // The format request takes a long time. When it completes, it inserts
+    // a newline and an indent before the `.`
+    cx.lsp
+        .handle_request::<lsp::request::Formatting, _, _>(move |_, cx| {
+            let executor = cx.background();
+            async move {
+                executor.timer(Duration::from_millis(100)).await;
+                Ok(Some(vec![lsp::TextEdit {
+                    range: lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(0, 3)),
+                    new_text: "\n    ".into(),
+                }]))
+            }
+        });
+
+    // Submit a format request.
+    let format_1 = cx
+        .update_editor(|editor, cx| editor.format(&Format, cx))
+        .unwrap();
+    cx.foreground().run_until_parked();
+
+    // Submit a second format request.
+    let format_2 = cx
+        .update_editor(|editor, cx| editor.format(&Format, cx))
+        .unwrap();
+    cx.foreground().run_until_parked();
+
+    // Wait for both format requests to complete
+    cx.foreground().advance_clock(Duration::from_millis(200));
+    cx.foreground().start_waiting();
+    format_1.await.unwrap();
+    cx.foreground().start_waiting();
+    format_2.await.unwrap();
+
+    // The formatting edits only happens once.
+    cx.assert_editor_state(indoc! {"
+        one
+            .twoˇ
+    "});
+}
+
 #[gpui::test]
 async fn test_completion(cx: &mut gpui::TestAppContext) {
     let mut cx = EditorLspTestContext::new_rust(

crates/project/src/project.rs

@@ -8,10 +8,7 @@ pub mod worktree;
 mod project_tests;
 
 use anyhow::{anyhow, Context, Result};
-use client::{
-    proto::{self},
-    Client, PeerId, TypedEnvelope, User, UserStore,
-};
+use client::{proto, Client, PeerId, TypedEnvelope, User, UserStore};
 use clock::ReplicaId;
 use collections::{hash_map, BTreeMap, HashMap, HashSet};
 use futures::{future::Shared, AsyncWriteExt, Future, FutureExt, StreamExt, TryFutureExt};
@@ -66,7 +63,7 @@ use std::{
     time::Instant,
 };
 use thiserror::Error;
-use util::{post_inc, ResultExt, TryFutureExt as _};
+use util::{defer, post_inc, ResultExt, TryFutureExt as _};
 
 pub use db::Db;
 pub use fs::*;
@@ -128,6 +125,7 @@ pub struct Project {
     opened_buffers: HashMap<u64, OpenBuffer>,
     incomplete_buffers: HashMap<u64, ModelHandle<Buffer>>,
     buffer_snapshots: HashMap<u64, Vec<(i32, TextBufferSnapshot)>>,
+    buffers_being_formatted: HashSet<usize>,
     nonce: u128,
     initialized_persistent_state: bool,
     _maintain_buffer_languages: Task<()>,
@@ -512,6 +510,7 @@ impl Project {
                 language_server_statuses: Default::default(),
                 last_workspace_edits_by_language_server: Default::default(),
                 language_server_settings: Default::default(),
+                buffers_being_formatted: Default::default(),
                 next_language_server_id: 0,
                 nonce: StdRng::from_entropy().gen(),
                 initialized_persistent_state: false,
@@ -627,6 +626,7 @@ impl Project {
                 last_workspace_edits_by_language_server: Default::default(),
                 next_language_server_id: 0,
                 opened_buffers: Default::default(),
+                buffers_being_formatted: Default::default(),
                 buffer_snapshots: Default::default(),
                 nonce: StdRng::from_entropy().gen(),
                 initialized_persistent_state: false,
@@ -3113,7 +3113,26 @@ impl Project {
                     .await?;
             }
 
-            for (buffer, buffer_abs_path, language_server) in local_buffers {
+            // Do not allow multiple concurrent formatting requests for the
+            // same buffer.
+            this.update(&mut cx, |this, _| {
+                local_buffers
+                    .retain(|(buffer, _, _)| this.buffers_being_formatted.insert(buffer.id()));
+            });
+            let _cleanup = defer({
+                let this = this.clone();
+                let mut cx = cx.clone();
+                let local_buffers = &local_buffers;
+                move || {
+                    this.update(&mut cx, |this, _| {
+                        for (buffer, _, _) in local_buffers {
+                            this.buffers_being_formatted.remove(&buffer.id());
+                        }
+                    });
+                }
+            });
+
+            for (buffer, buffer_abs_path, language_server) in &local_buffers {
                 let (format_on_save, formatter, tab_size) = buffer.read_with(&cx, |buffer, cx| {
                     let settings = cx.global::<Settings>();
                     let language_name = buffer.language().map(|language| language.name());
@@ -3165,7 +3184,7 @@ impl Project {
                             buffer.forget_transaction(transaction.id)
                         });
                     }
-                    project_transaction.0.insert(buffer, transaction);
+                    project_transaction.0.insert(buffer.clone(), transaction);
                 }
             }