From a52aa5febc44399923ace8a2b01070e0d2cd6b60 Mon Sep 17 00:00:00 2001
From: Andy Kipp <anki-code@users.noreply.github.com>
Date: Wed, 26 Jun 2024 08:32:51 +0200
Subject: [PATCH] Command cache: fix update cache logic (#5539)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

### Motivation

I noticed that when I type every letter in the prompt on the remote
server the typing is lagging. I thought it's ssh but no.
After inspection the commands cache code I found that on every key press
there is reading all 2000+ files in PATH because the code for cache
update has funny issue.

### Quiz

```xsh
def f():
    print(1)
    yield False
    print(2)
    yield True
    print(3)

print(any(f()))
```
What will be in the output? Answer:

<details>

```
1
2
True
```

The execution of `print(3)` ignored because `any()` interrupts the
execution of the function.
If we call `print(list(f()))` the output will be `1 2 3 [False, True]`.

</details>

### Before

* Updating cache (read all files in PATH and list of aliases) two times
after start.
* Updating cache (read all files in PATH and list of aliases) on every
key press in prompt.

### After

* Update cache once at start.
* Update cache only when we have real changes of paths or aliases.

cc #4954 #3895 #5309

## For community
⬇️ **Please click the 👍 reaction instead of leaving a `+1` or 👍
comment**

---------

Co-authored-by: a <1@1.1>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
---
 news/commands_cache_fix_update.rst | 23 +++++++++++++++++++++++
 tests/test_commands_cache.py       |  9 ++++++++-
 xonsh/commands_cache.py            | 23 +++++++++++++++--------
 3 files changed, 46 insertions(+), 9 deletions(-)
 create mode 100644 news/commands_cache_fix_update.rst

diff --git a/news/commands_cache_fix_update.rst b/news/commands_cache_fix_update.rst
new file mode 100644
index 000000000..3c8ad1d55
--- /dev/null
+++ b/news/commands_cache_fix_update.rst
@@ -0,0 +1,23 @@
+**Added:**
+
+* <news item>
+
+**Changed:**
+
+* <news item>
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* Commands Cache: Fixed cache update logic that lead to lagging during typing.
+
+**Security:**
+
+* <news item>
diff --git a/tests/test_commands_cache.py b/tests/test_commands_cache.py
index a586b9b02..bf3012dbd 100644
--- a/tests/test_commands_cache.py
+++ b/tests/test_commands_cache.py
@@ -187,9 +187,16 @@ def test_update_cache(xession, tmp_path):
     file1.touch()
     file1.chmod(0o755)
 
-    cache = CommandsCache({"PATH": [subdir2, subdir1]})
+    paths = [subdir2, subdir1]
+    cache = CommandsCache({"PATH": paths})
     cached = cache.update_cache()
 
+    # Check there are no changes after update cache.
+    c1 = cache._update_and_check_changes(paths)
+    c2 = cache._update_and_check_changes(paths)
+    c3 = cache._update_and_check_changes(paths)
+    assert [c1, c2, c3] == [True, False, False]
+
     assert file1.samefile(cached[basename][0])
 
     # give the os enough time to update the mtime field of the parent directory
diff --git a/xonsh/commands_cache.py b/xonsh/commands_cache.py
index 07cc046c1..8abdfb330 100644
--- a/xonsh/commands_cache.py
+++ b/xonsh/commands_cache.py
@@ -119,14 +119,21 @@ class CommandsCache(cabc.Mapping):
                 if os.path.isdir(p):
                     yield p
 
-    def _check_changes(self, paths: tuple[str, ...]):
-        # did PATH change?
-        yield self._update_paths_cache(paths)
+    def _update_aliases_cache(self):
+        """Update aliases checksum and return result: updated or not."""
+        prev_hash = self._alias_checksum
+        self._alias_checksum = hash(frozenset(self.aliases))
+        return prev_hash != self._alias_checksum
 
-        # did aliases change?
-        al_hash = hash(frozenset(self.aliases))
-        yield al_hash != self._alias_checksum
-        self._alias_checksum = al_hash
+    def _update_and_check_changes(self, paths: tuple[str, ...]):
+        """Update cache and return the result: updated or still the same.
+
+        Be careful in this place. Both `_update_*` functions must be called
+        because they are changing state after update.
+        """
+        is_aliases_change = self._update_aliases_cache()
+        is_paths_change = self._update_paths_cache(paths)
+        return is_aliases_change or is_paths_change
 
     @property
     def all_commands(self):
@@ -159,7 +166,7 @@ class CommandsCache(cabc.Mapping):
         # iterate backwards so that entries at the front of PATH overwrite
         # entries at the back.
         paths = tuple(reversed(tuple(self.remove_dups(env.get("PATH") or []))))
-        if any(self._check_changes(paths)):
+        if self._update_and_check_changes(paths):
             all_cmds = CacheDict()
             for cmd, path in self._iter_binaries(paths):
                 # None     -> not in aliases