Skip to content

Commit aa56078

Browse files
KenoKristofferC
authored andcommitted
test: replcompletions: Replace timedwait by proper condvar (#58643)
Replace fragile timing-based synchronization with proper condition variable signaling to ensure PATH cache updates complete before testing completions. This eliminates test flakiness on systems where the cache update takes longer than 5s. The test failure was seen in CI: https://buildkite.com/julialang/julia-master/builds/48273 (cherry picked from commit a4ab110)
1 parent 7c1ef00 commit aa56078

File tree

2 files changed

+44
-15
lines changed

2 files changed

+44
-15
lines changed

stdlib/REPL/src/REPLCompletions.jl

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,8 @@ end
330330

331331
const PATH_cache_lock = Base.ReentrantLock()
332332
const PATH_cache = Set{String}()
333-
PATH_cache_task::Union{Task,Nothing} = nothing # used for sync in tests
333+
PATH_cache_task::Union{Task,Nothing} = nothing
334+
PATH_cache_condition::Union{Threads.Condition, Nothing} = nothing # used for sync in tests
334335
next_cache_update::Float64 = 0.0
335336
function maybe_spawn_cache_PATH()
336337
global PATH_cache_task, next_cache_update
@@ -339,7 +340,11 @@ function maybe_spawn_cache_PATH()
339340
time() < next_cache_update && return
340341
PATH_cache_task = Threads.@spawn begin
341342
REPLCompletions.cache_PATH()
342-
@lock PATH_cache_lock PATH_cache_task = nothing # release memory when done
343+
@lock PATH_cache_lock begin
344+
next_cache_update = time() + 10 # earliest next update can run is 10s after
345+
PATH_cache_task = nothing # release memory when done
346+
PATH_cache_condition !== nothing && notify(PATH_cache_condition)
347+
end
343348
end
344349
Base.errormonitor(PATH_cache_task)
345350
end
@@ -350,8 +355,6 @@ function cache_PATH()
350355
path = get(ENV, "PATH", nothing)
351356
path isa String || return
352357

353-
global next_cache_update
354-
355358
# Calling empty! on PATH_cache would be annoying for async typing hints as completions would temporarily disappear.
356359
# 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.
357360
this_PATH_cache = Set{String}()
@@ -414,7 +417,6 @@ function cache_PATH()
414417

415418
@lock PATH_cache_lock begin
416419
intersect!(PATH_cache, this_PATH_cache) # remove entries from PATH_cache that weren't found this time
417-
next_cache_update = time() + 10 # earliest next update can run is 10s after
418420
end
419421

420422
@debug "caching PATH files took $t seconds" length(pathdirs) length(PATH_cache)

stdlib/REPL/test/replcompletions.jl

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1073,6 +1073,39 @@ let c, r, res
10731073
@test res === false
10741074
end
10751075

1076+
# A pair of utility function for the REPL completions test to test PATH_cache
1077+
# dependent completions, which ordinarily happen asynchronously.
1078+
# Only to be used from the test suite
1079+
function test_only_arm_cache_refresh()
1080+
@lock REPL.REPLCompletions.PATH_cache_lock begin
1081+
@assert REPL.REPLCompletions.PATH_cache_condition === nothing
1082+
1083+
# Arm a condition we can wait on
1084+
REPL.REPLCompletions.PATH_cache_condition = Threads.Condition(REPL.REPLCompletions.PATH_cache_lock)
1085+
1086+
# Check if the previous update is still running - if so, wait for it to finish
1087+
while REPL.REPLCompletions.PATH_cache_task !== nothing
1088+
@assert !istaskdone(REPL.REPLCompletions.PATH_cache_task)
1089+
wait(REPL.REPLCompletions.PATH_cache_condition)
1090+
end
1091+
1092+
# force the next cache update to happen immediately
1093+
REPL.REPLCompletions.next_cache_update = 0
1094+
end
1095+
return REPL.REPLCompletions.PATH_cache_condition
1096+
end
1097+
1098+
function test_only_wait_cache_path_done()
1099+
@lock REPL.REPLCompletions.PATH_cache_lock begin
1100+
@assert REPL.REPLCompletions.PATH_cache_condition !== nothing
1101+
1102+
while REPL.REPLCompletions.next_cache_update == 0.
1103+
wait(REPL.REPLCompletions.PATH_cache_condition)
1104+
end
1105+
REPL.REPLCompletions.PATH_cache_condition = nothing
1106+
end
1107+
end
1108+
10761109
if Sys.isunix()
10771110
let s, c, r
10781111
#Assume that we can rely on the existence and accessibility of /tmp
@@ -1204,12 +1237,9 @@ let s, c, r
12041237
# Files reachable by PATH are cached async when PATH is seen to have been changed by `complete_path`
12051238
# so changes are unlikely to appear in the first complete. For testing purposes we can wait for
12061239
# caching to finish
1207-
@lock REPL.REPLCompletions.PATH_cache_lock begin
1208-
# force the next cache update to happen immediately
1209-
REPL.REPLCompletions.next_cache_update = 0
1210-
end
1240+
test_only_arm_cache_refresh()
12111241
c,r = test_scomplete(s)
1212-
timedwait(()->REPL.REPLCompletions.next_cache_update != 0, 5) # wait for caching to complete
1242+
test_only_wait_cache_path_done()
12131243
c,r = test_scomplete(s)
12141244
@test "tmp-executable" in c
12151245
@test r == 1:9
@@ -1238,12 +1268,9 @@ let s, c, r
12381268

12391269
withenv("PATH" => string(tempdir(), ":", dir)) do
12401270
s = string("repl-completio")
1241-
@lock REPL.REPLCompletions.PATH_cache_lock begin
1242-
# force the next cache update to happen immediately
1243-
REPL.REPLCompletions.next_cache_update = 0
1244-
end
1271+
test_only_arm_cache_refresh()
12451272
c,r = test_scomplete(s)
1246-
timedwait(()->REPL.REPLCompletions.next_cache_update != 0, 5) # wait for caching to complete
1273+
test_only_wait_cache_path_done()
12471274
c,r = test_scomplete(s)
12481275
@test ["repl-completion"] == c
12491276
@test s[r] == "repl-completio"

0 commit comments

Comments
 (0)