Replace PATH cache update synchronization in REPLCompletions tests #59161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just block on REPLCompletions.cache_PATH() instead, which takes the appropriate lock to safely update the cache already. Co-authored-by: Gabriel Baraldi <[email protected]>
gbaraldi
approved these changes
Jul 30, 2025
Keno
reviewed
Jul 30, 2025
| withenv("PATH" => string(tempdir(), ":", dir)) do | ||
| s = string("repl-completio") | ||
| test_only_arm_cache_refresh() | ||
| c,r = test_scomplete(s) |
There was a problem hiding this comment.
This defeats the purpose of this test.
Keno
added a commit
that referenced
this pull request
Jul 31, 2025
This is an alternative to #59161, attempting to fix the same observed CI behavior. I don't think #59161 is the best way to fix it, as the point of these tests is to make sure that REPL completions looks up the PATH internally. Calling the path update function explicitly defeats that somewhat. The extra synchronization here to make this deterministic is messy, but I do think it makes the test closer to real-world usage. The core attempted fix here is to move the read of the PATH_ locals inside `maybe_spawn_cache_PATH` into the locked region. If they are not under the lock, they could be unconditionally overwritten by a second call to this function, causing issues in the state machine. I do not know whether this is the cause of the observed CI hangs, but it's worth fixing anyway.
Keno
added a commit
that referenced
this pull request
Jul 31, 2025
This is an alternative to #59161, attempting to fix the same observed CI behavior. I don't think #59161 is the best way to fix it, as the point of these tests is to make sure that REPL completions looks up the PATH internally. Calling the path update function explicitly defeats that somewhat. The extra synchronization here to make this deterministic is messy, but I do think it makes the test closer to real-world usage. The core attempted fix here is to move the read of the PATH_ locals inside `maybe_spawn_cache_PATH` into the locked region. If they are not under the lock, they could be unconditionally overwritten by a second call to this function, causing issues in the state machine. I do not know whether this is the cause of the observed CI hangs, but it's worth fixing anyway.
|
#59166 for my preferred alternative |
Keno
added a commit
that referenced
this pull request
Jul 31, 2025
This is an alternative to #59161, attempting to fix the same observed CI behavior. I don't think #59161 is the best way to fix it, as the point of these tests is to make sure that REPL completions looks up the PATH internally. Calling the path update function explicitly defeats that somewhat. The extra synchronization here to make this deterministic is messy, but I do think it makes the test closer to real-world usage. The core attempted fix here is to move the read of the PATH_ locals inside `maybe_spawn_cache_PATH` into the locked region. If they are not under the lock, they could be unconditionally overwritten by a second call to this function, causing issues in the state machine. I do not know whether this is the cause of the observed CI hangs, but it's worth fixing anyway.
KristofferC
pushed a commit
that referenced
this pull request
Sep 5, 2025
This is an alternative to #59161, attempting to fix the same observed CI behavior. I don't think #59161 is the best way to fix it, as the point of these tests is to make sure that REPL completions looks up the PATH internally. Calling the path update function explicitly defeats that somewhat. The extra synchronization here to make this deterministic is messy, but I do think it makes the test closer to real-world usage. The core attempted fix here is to move the read of the PATH_ locals inside `maybe_spawn_cache_PATH` into the locked region. If they are not under the lock, they could be unconditionally overwritten by a second call to this function, causing issues in the state machine. I do not know whether this is the cause of the observed CI hangs, but it's worth fixing anyway. (cherry picked from commit 9a16119)
KristofferC
pushed a commit
that referenced
this pull request
Sep 15, 2025
This is an alternative to #59161, attempting to fix the same observed CI behavior. I don't think #59161 is the best way to fix it, as the point of these tests is to make sure that REPL completions looks up the PATH internally. Calling the path update function explicitly defeats that somewhat. The extra synchronization here to make this deterministic is messy, but I do think it makes the test closer to real-world usage. The core attempted fix here is to move the read of the PATH_ locals inside `maybe_spawn_cache_PATH` into the locked region. If they are not under the lock, they could be unconditionally overwritten by a second call to this function, causing issues in the state machine. I do not know whether this is the cause of the observed CI hangs, but it's worth fixing anyway. (cherry picked from commit 9a16119)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempt to fix flakiness (https://buildkite.com/julialang/julia-master/builds/49532#01985bab-e15a-4efb-81a4-d53fdae669d4) by blocking on
REPLCompletions.cache_PATH()in the tests that rely on it.