Skip to content
This repository has been archived by the owner on Apr 29, 2024. It is now read-only.

Sometimes, highlighting doesn’t happen and some weird hooks are required to trigger them #223

Open
hadronized opened this issue Feb 10, 2024 · 7 comments
Labels
bug Something isn't working cant-reproduce The issue is not reproducible TBD To Be Determined

Comments

@hadronized
Copy link
Owner

I’m not sure about this one, so hard to reproduce. It happens at work from time to time, especially after hibernating / sleep. KTS either doesn’t send the responses fast enough, or hooks are slow to ask KTS to highlight, or maybe a mix between both.

I do not see any potential bottlenecks, so I need to investigate a bit more.

@hadronized hadronized added bug Something isn't working cant-reproduce The issue is not reproducible TBD To Be Determined labels Feb 10, 2024
@robem
Copy link
Contributor

robem commented Apr 14, 2024

Not sure if I encounter the same issue but just scrolling through a file (line by line) seems to trigger an update and 1/5 times the highlighting is messed up. When pressing random move keys thereafter the triggered update succeeds eventually. However, this makes kak-tree-sitter unusable at the moment for me. I will see if applying #232 helps.

@robem
Copy link
Contributor

robem commented Apr 14, 2024

After applying #240, the updating issues go away. Well, it is probably still present but the reduced update rate leads to triggering this bug less frequently.

@hadronized
Copy link
Owner Author

Yes, I plan on integrating something to prevent sending the buffer all the time, but #240 won’t cut it as I plan on changing the streaming model ~soon.

@dsabadie-datadog
Copy link

Looks like a quickfix is to just remove the initial req-enable when returning the initial response… I used to add that because of the mess that hooks are, but maybe it’s not needed anymore. I’ll roll it with that patch as experimental for now.

@robem
Copy link
Contributor

robem commented Apr 18, 2024

Didn't work for me. I added some debug statements and found that highlighting is wrong whenever the read buffer size doesn't equal the actual buffer size. Meaning, when running file.read_to_string(buffer) on the buffer fifo, then we sometimes do partial reads (or there is a write issue or an internal fifo quirk). Fsync and similar tools shouldn't matter given that it is a fifo. Also, because the fifo is open permanently, we cannot read until the end.
An option could be to send the buffer size with the highlight command and then the following fifo buffer read reads as many bytes as specified. I haven't tested that.
Update: My newest theory is that kakoune truncates the write if the fifo is full. This is supported by the fact that I see the buffer size difference with large files only (e.g. src/server.rs). Meaning, the producer is faster than the consumer. We could try to read immediately from the buffer and continue reading until we hit a 'WouldBlock' instead of waiting for the event.
In addition, I ran ulimit on my Mac to increase buffer sizes without noticable success.

@robem
Copy link
Contributor

robem commented Apr 19, 2024

The following diff avoids garbled highlights because the full buffer is being read on the kts side. The issues remaining is that sometimes highlighting is randomly toggled.

diff --git a/kak-tree-sitter/src/server.rs b/kak-tree-sitter/src/server.rs
index b0c28f8..7715904 100644
--- a/kak-tree-sitter/src/server.rs
+++ b/kak-tree-sitter/src/server.rs
@@ -868,17 +868,25 @@ impl FifoHandler {
       session_name = session.name()
     );

-    if let Err(err) = file.read_to_string(buffer) {
-      if err.kind() == io::ErrorKind::WouldBlock {
-        log::debug!("buffer FIFO is not ready");
-        return Ok(());
-      } else {
-        return Err(OhNo::InvalidRequest {
-          req: "<buf>".to_owned(),
-          err: err.to_string(),
-        });
+    let mut tmp = String::new();
+    loop {
+      match file.read_to_string(&mut tmp) {
+        Ok(0) => break,
+        Ok(_) => buffer.push_str(&tmp),
+        Err(err) => {
+          if err.kind() == io::ErrorKind::WouldBlock {
+            log::debug!("buffer FIFO is not ready");
+            continue;
+            // return Ok(());
+          } else {
+            return Err(OhNo::InvalidRequest {
+              req: "<buf>".to_owned(),
+              err: err.to_string(),
+            });
+          }
+        }
       }
-    };
+    }

     let res = self.process_buf(session, buffer);
     buffer.clear();

@hadronized
Copy link
Owner Author

Interesting, we could probably merge such a fix. Thank for looking into it!

hadronized added a commit that referenced this issue Apr 24, 2024
Relates #223, but not sure it fixes it. I want to redesign the whole buffer streaming (#249) so this is a temporary fix
until we got something more robust — along with lazy commands (#240).

Thanks [@robem](#223 (comment)) for suggesting the
fix.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working cant-reproduce The issue is not reproducible TBD To Be Determined
Projects
None yet
Development

No branches or pull requests

3 participants