REPLCompletions: PATH caching tweaks

stdlib/REPL/src/REPLCompletions.jl

@@ -271,25 +271,34 @@ end
 
 const PATH_cache_lock = Base.ReentrantLock()
 const PATH_cache = Set{String}()
-cached_PATH_string::Union{String,Nothing} = nothing
-function cached_PATH_changed()
-    global cached_PATH_string
-    @lock(PATH_cache_lock, cached_PATH_string) !== get(ENV, "PATH", nothing)
+PATH_cache_task::Union{Task,Nothing} = nothing # used for sync in tests
+next_cache_update::Float64 = 0.0
+function maybe_spawn_cache_PATH()
+    global PATH_cache_task, next_cache_update
+    @lock PATH_cache_lock begin
+        PATH_cache_task isa Task && !istaskdone(PATH_cache_task) && return
+        time() < next_cache_update && return
+        PATH_cache_task = Threads.@spawn REPLCompletions.cache_PATH()
+        Base.errormonitor(PATH_cache_task)
+    end
 end
-const PATH_cache_finished = Base.Condition() # used for sync in tests
 
 # caches all reachable files in PATH dirs
 function cache_PATH()
-    global cached_PATH_string
-    path = @lock PATH_cache_lock begin
-        empty!(PATH_cache)
-        cached_PATH_string = get(ENV, "PATH", nothing)
-    end
+    path = get(ENV, "PATH", nothing)
     path isa String || return
 
+    global next_cache_update
+
+    # Calling empty! on PATH_cache would be annoying for async typing hints as completions would temporarily disappear.
+    # So keep track of what's added this time and at the end remove any that didn't appear this time from the global cache.
+    this_PATH_cache = Set{String}()
+
     @debug "caching PATH files" PATH=path
     pathdirs = split(path, @static Sys.iswindows() ? ";" : ":")
 
+    next_yield_time = time() + 0.01
+
     t = @elapsed for pathdir in pathdirs
         actualpath = try
             realpath(pathdir)
@@ -322,6 +331,7 @@ function cache_PATH()
             try
                 if isfile(joinpath(pathdir, file))
                     @lock PATH_cache_lock push!(PATH_cache, file)
+                    push!(this_PATH_cache, file)
                 end
             catch e
                 # `isfile()` can throw in rare cases such as when probing a
@@ -333,10 +343,18 @@ function cache_PATH()
                     rethrow()
                 end
             end
-            yield() # so startup doesn't block when -t1
+            if time() >= next_yield_time
+                yield() # to avoid blocking typing when -t1
+                next_yield_time = time() + 0.01
+            end
         end
     end
-    notify(PATH_cache_finished)
+
+    @lock PATH_cache_lock begin
+        intersect!(PATH_cache, this_PATH_cache) # remove entries from PATH_cache that weren't found this time
+        next_cache_update = time() + 10 # earliest next update can run is 10s after
+    end
+
     @debug "caching PATH files took $t seconds" length(pathdirs) length(PATH_cache)
     return PATH_cache
 end
@@ -380,15 +398,13 @@ function complete_path(path::AbstractString;
     end
 
     if use_envpath && isempty(dir)
-        # Look for files in PATH as well. These are cached in `cache_PATH` in a separate task in REPL init.
-        # If we cannot get lock because its still caching just pass over this so that initial
-        # typing isn't laggy. If the PATH string has changed since last cache re-cache it
-        cached_PATH_changed() && Base.errormonitor(Threads.@spawn REPLCompletions.cache_PATH())
-        if trylock(PATH_cache_lock)
+        # Look for files in PATH as well. These are cached in `cache_PATH` in an async task to not block typing.
+        # If we cannot get lock because its still caching just pass over this so that typing isn't laggy.
+        maybe_spawn_cache_PATH() # only spawns if enough time has passed and the previous caching task has completed
+        @lock PATH_cache_lock begin
             for file in PATH_cache
                 startswith(file, prefix) && push!(matches, file)
             end
-            unlock(PATH_cache_lock)
         end
     end
 

stdlib/REPL/test/replcompletions.jl

@@ -1146,11 +1146,15 @@ let s, c, r
             # PATH can also contain folders which we aren't actually allowed to read.
             withenv("PATH" => string(path, ":", unreadable)) do
                 s = "tmp-execu"
-                c,r = test_scomplete(s)
                 # Files reachable by PATH are cached async when PATH is seen to have been changed by `complete_path`
                 # so changes are unlikely to appear in the first complete. For testing purposes we can wait for
                 # caching to finish
-                wait(REPL.REPLCompletions.PATH_cache_finished)
+                @lock REPL.REPLCompletions.PATH_cache_lock begin
+                    # force the next cache update to happen immediately
+                    REPL.REPLCompletions.next_cache_update = 0
+                end
+                c,r = test_scomplete(s)
+                wait(REPL.REPLCompletions.PATH_cache_task::Task) # wait for caching to complete
                 c,r = test_scomplete(s)
                 @test "tmp-executable" in c
                 @test r == 1:9
@@ -1179,8 +1183,12 @@ let s, c, r
 
             withenv("PATH" => string(tempdir(), ":", dir)) do
                 s = string("repl-completio")
+                @lock REPL.REPLCompletions.PATH_cache_lock begin
+                    # force the next cache update to happen immediately
+                    REPL.REPLCompletions.next_cache_update = 0
+                end
                 c,r = test_scomplete(s)
-                wait(REPL.REPLCompletions.PATH_cache_finished) # wait for caching to complete
+                wait(REPL.REPLCompletions.PATH_cache_task::Task) # wait for caching to complete
                 c,r = test_scomplete(s)
                 @test ["repl-completion"] == c
                 @test s[r] == "repl-completio"