address review comments and ensure lock is always aquired

Author: pascalkuthe

helix-term/src/application.rs

@@ -284,7 +284,7 @@ impl Application {
             scroll: None,
         };
 
-        // Aquire mutable access to the redraw_handle lock
+        // Acquire mutable access to the redraw_handle lock
         // to ensure that there are no tasks running that want to block rendering
         drop(cx.editor.redraw_handle.1.write().await);
         cx.editor.needs_redraw = false;

helix-vcs/src/diff.rs

@@ -5,34 +5,33 @@ use helix_core::Rope;
 use imara_diff::Algorithm;
 use parking_lot::{Mutex, MutexGuard};
 use tokio::sync::mpsc::{unbounded_channel, UnboundedSender};
-use tokio::sync::{Notify, RwLock};
+use tokio::sync::{Notify, OwnedRwLockReadGuard, RwLock};
 use tokio::task::JoinHandle;
+use tokio::time::Instant;
 
 use crate::diff::worker::DiffWorker;
 
 mod line_cache;
 mod worker;
 
-type RedrawHandle = Arc<(Notify, RwLock<()>)>;
+type RedrawHandle = (Arc<Notify>, Arc<RwLock<()>>);
 
-// The order of enum variants is used by the PartialOrd
-// derive macro, DO NOT REORDER
-#[derive(PartialEq, PartialOrd)]
-enum RenderStrategy {
-    Async,
-    SyncWithTimeout,
-    Sync,
+/// A rendering lock passed to the differ the prevents redraws from occurring
+struct RenderLock {
+    pub lock: OwnedRwLockReadGuard<()>,
+    pub timeout: Option<Instant>,
 }
 
 struct Event {
     text: Rope,
     is_base: bool,
-    render_strategy: RenderStrategy,
+    render_lock: Option<RenderLock>,
 }
 
 #[derive(Clone, Debug)]
 pub struct DiffHandle {
     channel: UnboundedSender<Event>,
+    render_lock: Arc<RwLock<()>>,
     hunks: Arc<Mutex<Vec<Hunk>>>,
     inverted: bool,
 }
@@ -53,14 +52,15 @@ impl DiffHandle {
             channel: receiver,
             hunks: hunks.clone(),
             new_hunks: Vec::default(),
-            redraw_handle,
-            difff_finished_notify: Arc::default(),
+            redraw_notify: redraw_handle.0,
+            diff_finished_notify: Arc::default(),
         };
         let handle = tokio::spawn(worker.run(diff_base, doc));
         let differ = DiffHandle {
             channel: sender,
             hunks,
             inverted: false,
+            render_lock: redraw_handle.1,
         };
         (differ, handle)
     }
@@ -76,36 +76,48 @@ impl DiffHandle {
         }
     }
 
+    /// Updates the document associated with this redraw handle
+    /// This function is only intended to be called from within the rendering loop
+    /// if called from elsewhere it may fail to acquire the render lock and panic
     pub fn update_document(&self, doc: Rope, block: bool) -> bool {
-        let mode = if block {
-            RenderStrategy::Sync
+        // unwrap is ok here because the rendering lock is
+        // only exclusively locked during redraw.
+        // This function is only intended to be called
+        // from the core rendering loop where no redraw can happen in parallel
+        let lock = self.render_lock.clone().try_read_owned().unwrap();
+        let timeout = if block {
+            None
         } else {
-            RenderStrategy::SyncWithTimeout
+            Some(Instant::now() + tokio::time::Duration::from_millis(SYNC_DIFF_TIMEOUT))
         };
-        self.update_document_impl(doc, self.inverted, mode)
+        self.update_document_impl(doc, self.inverted, Some(RenderLock { lock, timeout }))
     }
 
     pub fn update_diff_base(&self, diff_base: Rope) -> bool {
-        self.update_document_impl(diff_base, !self.inverted, RenderStrategy::Async)
+        self.update_document_impl(diff_base, !self.inverted, None)
     }
 
-    fn update_document_impl(&self, text: Rope, is_base: bool, mode: RenderStrategy) -> bool {
+    fn update_document_impl(
+        &self,
+        text: Rope,
+        is_base: bool,
+        render_lock: Option<RenderLock>,
+    ) -> bool {
         let event = Event {
             text,
             is_base,
-            render_strategy: mode,
+            render_lock,
         };
         self.channel.send(event).is_ok()
     }
 }
 
-// TODO configuration
 /// synchronous debounce value should be low
 /// so we can update synchronously most of the time
 const DIFF_DEBOUNCE_TIME_SYNC: u64 = 1;
 /// maximum time that rendering should be blocked until the diff finishes
-const SYNC_DIFF_TIMEOUT: u64 = 50;
-const DIFF_DEBOUNCE_TIME_ASYNC: u64 = 100;
+const SYNC_DIFF_TIMEOUT: u64 = 12;
+const DIFF_DEBOUNCE_TIME_ASYNC: u64 = 96;
 const ALGORITHM: Algorithm = Algorithm::Histogram;
 const MAX_DIFF_LINES: usize = 64 * u16::MAX as usize;
 // cap average line length to 128 for files with MAX_DIFF_LINES
@@ -128,15 +140,15 @@ pub struct Hunk {
 
 impl Hunk {
     /// Can be used instead of `Option::None` for better performance
-    /// because lines larger than `i32::MAX` are not supported by imara-diff anways.
-    /// Has some nice properties where it usually is not necessary to check for `None` seperatly:
-    /// Empty ranges fail contains checks and also fails smaller than checks.
+    /// because lines larger then `i32::MAX` are not supported by `imara-diff` anyways.
+    /// Has some nice properties where it usually is not necessary to check for `None` separately:
+    /// Empty ranges fail contains checks and also fails smaller then checks.
     pub const NONE: Hunk = Hunk {
         before: u32::MAX..u32::MAX,
         after: u32::MAX..u32::MAX,
     };
 
-    /// Inverts a change so that `before` becomes `after` and `after` becomes `before`
+    /// Inverts a change so that `before`
     pub fn invert(&self) -> Hunk {
         Hunk {
             before: self.after.clone(),

helix-vcs/src/diff/worker.rs

@@ -6,12 +6,11 @@ use helix_core::{Rope, RopeSlice};
 use imara_diff::intern::InternedInput;
 use parking_lot::Mutex;
 use tokio::sync::mpsc::UnboundedReceiver;
-use tokio::sync::{Notify, RwLockReadGuard};
-use tokio::time::{timeout, timeout_at, Duration, Instant};
+use tokio::sync::Notify;
+use tokio::time::{timeout, timeout_at, Duration};
 
 use crate::diff::{
-    Event, RedrawHandle, RenderStrategy, ALGORITHM, DIFF_DEBOUNCE_TIME_ASYNC,
-    DIFF_DEBOUNCE_TIME_SYNC, SYNC_DIFF_TIMEOUT,
+    Event, RenderLock, ALGORITHM, DIFF_DEBOUNCE_TIME_ASYNC, DIFF_DEBOUNCE_TIME_SYNC,
 };
 
 use super::line_cache::InternedRopeLines;
@@ -24,19 +23,19 @@ pub(super) struct DiffWorker {
     pub channel: UnboundedReceiver<Event>,
     pub hunks: Arc<Mutex<Vec<Hunk>>>,
     pub new_hunks: Vec<Hunk>,
-    pub redraw_handle: RedrawHandle,
-    pub difff_finished_notify: Arc<Notify>,
+    pub redraw_notify: Arc<Notify>,
+    pub diff_finished_notify: Arc<Notify>,
 }
 
 impl DiffWorker {
     async fn accumulate_events(&mut self, event: Event) -> (Option<Rope>, Option<Rope>) {
-        let mut accumulator = EventAccumulator::new(&self.redraw_handle);
+        let mut accumulator = EventAccumulator::new();
         accumulator.handle_event(event).await;
         accumulator
             .accumulate_debounced_events(
                 &mut self.channel,
-                self.redraw_handle.clone(),
-                self.difff_finished_notify.clone(),
+                self.redraw_notify.clone(),
+                self.diff_finished_notify.clone(),
             )
             .await;
         (accumulator.doc, accumulator.diff_base)
@@ -80,7 +79,7 @@ impl DiffWorker {
     /// To improve performance this function tries to reuse the allocation of the old diff previously stored in `self.line_diffs`
     fn apply_hunks(&mut self) {
         swap(&mut *self.hunks.lock(), &mut self.new_hunks);
-        self.difff_finished_notify.notify_waiters();
+        self.diff_finished_notify.notify_waiters();
         self.new_hunks.clear();
     }
 
@@ -91,24 +90,18 @@ impl DiffWorker {
     }
 }
 
-struct EventAccumulator<'a> {
+struct EventAccumulator {
     diff_base: Option<Rope>,
     doc: Option<Rope>,
-    render_stratagey: RenderStrategy,
-    redraw_handle: &'a RedrawHandle,
-    render_lock: Option<RwLockReadGuard<'a, ()>>,
-    timeout: Instant,
+    render_lock: Option<RenderLock>,
 }
 
-impl<'a> EventAccumulator<'a> {
-    fn new(redraw_handle: &'a RedrawHandle) -> EventAccumulator<'a> {
+impl<'a> EventAccumulator {
+    fn new() -> EventAccumulator {
         EventAccumulator {
             diff_base: None,
             doc: None,
-            render_stratagey: RenderStrategy::Async,
             render_lock: None,
-            redraw_handle,
-            timeout: Instant::now(),
         }
     }
 
@@ -122,25 +115,33 @@ impl<'a> EventAccumulator<'a> {
         *dst = Some(event.text);
 
         // always prefer the most synchronous requested render mode
-        if event.render_strategy > self.render_stratagey {
-            if self.render_lock.is_none() {
-                self.timeout = Instant::now() + Duration::from_millis(SYNC_DIFF_TIMEOUT);
-                self.render_lock = Some(self.redraw_handle.1.read().await);
+        if let Some(render_lock) = event.render_lock {
+            match &mut self.render_lock {
+                Some(RenderLock { timeout, .. }) => {
+                    // A timeout of `None` means that the render should
+                    // always wait for the diff to complete (so no timeout)
+                    // remove the existing timeout, otherwise keep the previous timeout
+                    // because it will be shorter then the current timeout
+                    if render_lock.timeout.is_none() {
+                        timeout.take();
+                    }
+                }
+                None => self.render_lock = Some(render_lock),
             }
-            self.render_stratagey = event.render_strategy
         }
     }
 
     async fn accumulate_debounced_events(
         &mut self,
         channel: &mut UnboundedReceiver<Event>,
-        redraw_handle: RedrawHandle,
+        redraw_notify: Arc<Notify>,
         diff_finished_notify: Arc<Notify>,
     ) {
         let async_debounce = Duration::from_millis(DIFF_DEBOUNCE_TIME_ASYNC);
         let sync_debounce = Duration::from_millis(DIFF_DEBOUNCE_TIME_SYNC);
         loop {
-            let debounce = if self.render_stratagey == RenderStrategy::Async {
+            // if we are not blocking rendering use a much longer timeout
+            let debounce = if self.render_lock.is_none() {
                 async_debounce
             } else {
                 sync_debounce
@@ -154,24 +155,30 @@ impl<'a> EventAccumulator<'a> {
         }
 
         // setup task to trigger the rendering
-        // with the chosen render stragey
-        match self.render_stratagey {
-            RenderStrategy::Async => {
+        match self.render_lock.take() {
+            // diff is performed outside of the rendering loop
+            // request a redraw after the diff is done
+            None => {
                 tokio::spawn(async move {
                     diff_finished_notify.notified().await;
-                    redraw_handle.0.notify_one();
+                    redraw_notify.notify_one();
                 });
             }
-            RenderStrategy::SyncWithTimeout => {
-                let timeout = self.timeout;
+            // diff is performed inside the rendering loop
+            // block redraw until the diff is done or the timeout is expired
+            Some(RenderLock {
+                lock,
+                timeout: Some(timeout),
+            }) => {
                 tokio::spawn(async move {
                     let res = {
                         // Acquire a lock on the redraw handle.
                         // The lock will block the rendering from occurring while held.
                         // The rendering waits for the diff if it doesn't time out
-                        let _render_guard = redraw_handle.1.read();
                         timeout_at(timeout, diff_finished_notify.notified()).await
                     };
+                    // we either reached the timeout or the diff is finished, release the render lock
+                    drop(lock);
                     if res.is_ok() {
                         // Diff finished in time we are done.
                         return;
@@ -180,15 +187,19 @@ impl<'a> EventAccumulator<'a> {
                     // and wait until the diff occurs to trigger an async redraw
                     log::warn!("Diff computation timed out, update of diffs might appear delayed");
                     diff_finished_notify.notified().await;
-                    redraw_handle.0.notify_one();
+                    redraw_notify.notify_one();
                 });
             }
-            RenderStrategy::Sync => {
+            // a blocking diff is performed inside the rendering loop
+            // block redraw until the diff is done
+            Some(RenderLock {
+                lock,
+                timeout: None,
+            }) => {
                 tokio::spawn(async move {
-                    // Aquire a lock on the redraw handle.
-                    // The lock will block the rendering from occuring while held.
-                    let _render_guard = redraw_handle.1.read();
-                    diff_finished_notify.notified().await
+                    diff_finished_notify.notified().await;
+                    // diff is done release the lock
+                    drop(lock)
                 });
             }
         };

helix-vcs/src/diff/worker/test.rs

@@ -1,5 +1,3 @@
-use std::sync::Arc;
-
 use helix_core::Rope;
 use tokio::task::JoinHandle;
 
@@ -10,7 +8,7 @@ impl DiffHandle {
         DiffHandle::new_with_handle(
             Rope::from_str(diff_base),
             Rope::from_str(doc),
-            Arc::default(),
+            Default::default(),
         )
     }
     async fn into_diff(self, handle: JoinHandle<()>) -> Vec<Hunk> {

helix-view/src/editor.rs

@@ -154,7 +154,7 @@ pub struct Config {
     )]
     pub idle_timeout: Duration,
     /// Time in milliseconds since last keypress before a redraws trigger.
-    /// Used for redrawing asynchronsouly computed UI components, set to 0 for instant.
+    /// Used for redrawing asynchronously computed UI components, set to 0 for instant.
     /// Defaults to 100ms.
     #[serde(
         serialize_with = "serialize_duration_millis",
@@ -733,11 +733,11 @@ pub struct Editor {
     /// Allows asynchronous tasks to control the rendering
     /// The `Notify` allows asynchronous tasks to request the editor to perform a redraw
     /// The `RwLock` blocks the editor from performing the render until an exclusive lock can be aquired
-    pub redraw_handle: Arc<(Notify, RwLock<()>)>,
+    pub redraw_handle: RedrawHandle,
     pub needs_redraw: bool,
 }
 
-pub type RedrawHandle = Arc<(Notify, RwLock<()>)>;
+pub type RedrawHandle = (Arc<Notify>, Arc<RwLock<()>>);
 
 #[derive(Debug)]
 pub enum EditorEvent {
@@ -830,7 +830,7 @@ impl Editor {
             auto_pairs,
             exit_code: 0,
             config_events: unbounded_channel(),
-            redraw_handle: Arc::default(),
+            redraw_handle: Default::default(),
             needs_redraw: false,
         }
     }