Skip to content

Commit 4fb861c

Browse files
karthiknadigCopilot
andcommitted
fix: release find configuration lock before discovery (Fixes #543)
Capture only the environment directory list before workspace traversal. Extract synchronous find execution for regression tests proving a writer can proceed during blocked discovery without changing the active directory snapshot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3df09ea commit 4fb861c

2 files changed

Lines changed: 199 additions & 43 deletions

File tree

‎crates/pet/src/jsonrpc.rs‎

Lines changed: 193 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1463,53 +1463,57 @@ pub struct FindOptions {
14631463
pub search_path: PathBuf,
14641464
}
14651465

1466+
fn execute_find(context: &Context, find_options: &FindOptions) -> Vec<PythonEnvironment> {
1467+
let now = Instant::now();
1468+
trace!("Finding environments in {:?}", find_options.search_path);
1469+
let global_env_search_paths: Vec<PathBuf> =
1470+
get_search_paths_from_env_variables(context.os_environment.as_ref());
1471+
1472+
let collect_reporter = Arc::new(collect::create_reporter());
1473+
let reporter = CacheReporter::new(collect_reporter.clone());
1474+
if find_options.search_path.is_file() {
1475+
identify_python_executables_using_locators(
1476+
vec![find_options.search_path.clone()],
1477+
&context.locators,
1478+
&reporter,
1479+
&global_env_search_paths,
1480+
);
1481+
} else {
1482+
let environment_directories = context
1483+
.configuration
1484+
.read()
1485+
.expect("configuration lock poisoned")
1486+
.config
1487+
.environment_directories
1488+
.clone();
1489+
find_python_environments_in_workspace_folder_recursive(
1490+
&find_options.search_path,
1491+
&reporter,
1492+
&context.locators,
1493+
&global_env_search_paths,
1494+
environment_directories.as_deref().unwrap_or(&[]),
1495+
);
1496+
}
1497+
1498+
let envs = collect_reporter
1499+
.environments
1500+
.lock()
1501+
.expect("environments mutex poisoned")
1502+
.clone();
1503+
trace!(
1504+
"Find completed in {:?}, found {} environments in {:?}",
1505+
now.elapsed(),
1506+
envs.len(),
1507+
find_options.search_path
1508+
);
1509+
envs
1510+
}
1511+
14661512
pub fn handle_find(context: Arc<Context>, id: u32, params: Value) {
14671513
thread::spawn(
14681514
move || match serde_json::from_value::<FindOptions>(params.clone()) {
14691515
Ok(find_options) => {
1470-
let now = Instant::now();
1471-
trace!("Finding environments in {:?}", find_options.search_path);
1472-
let global_env_search_paths: Vec<PathBuf> =
1473-
get_search_paths_from_env_variables(context.os_environment.as_ref());
1474-
1475-
let collect_reporter = Arc::new(collect::create_reporter());
1476-
let reporter = CacheReporter::new(collect_reporter.clone());
1477-
if find_options.search_path.is_file() {
1478-
identify_python_executables_using_locators(
1479-
vec![find_options.search_path.clone()],
1480-
&context.locators,
1481-
&reporter,
1482-
&global_env_search_paths,
1483-
);
1484-
} else {
1485-
find_python_environments_in_workspace_folder_recursive(
1486-
&find_options.search_path,
1487-
&reporter,
1488-
&context.locators,
1489-
&global_env_search_paths,
1490-
context
1491-
.configuration
1492-
.read()
1493-
.unwrap()
1494-
.config
1495-
.clone()
1496-
.environment_directories
1497-
.as_deref()
1498-
.unwrap_or(&[]),
1499-
);
1500-
}
1501-
1502-
let envs = collect_reporter
1503-
.environments
1504-
.lock()
1505-
.expect("environments mutex poisoned")
1506-
.clone();
1507-
trace!(
1508-
"Find completed in {:?}, found {} environments in {:?}",
1509-
now.elapsed(),
1510-
envs.len(),
1511-
find_options.search_path
1512-
);
1516+
let envs = execute_find(&context, &find_options);
15131517
if envs.is_empty() {
15141518
send_reply(id, None::<Vec<PythonEnvironment>>);
15151519
} else {
@@ -1805,6 +1809,152 @@ mod tests {
18051809
})
18061810
}
18071811

1812+
struct BlockingFindLocator {
1813+
blocked_executable: PathBuf,
1814+
started: mpsc::Sender<()>,
1815+
release: Mutex<mpsc::Receiver<()>>,
1816+
}
1817+
1818+
impl Locator for BlockingFindLocator {
1819+
fn get_kind(&self) -> LocatorKind {
1820+
LocatorKind::Venv
1821+
}
1822+
1823+
fn supported_categories(&self) -> Vec<PythonEnvironmentKind> {
1824+
vec![PythonEnvironmentKind::Venv]
1825+
}
1826+
1827+
fn try_from(&self, env: &pet_core::env::PythonEnv) -> Option<PythonEnvironment> {
1828+
if env.executable == self.blocked_executable {
1829+
self.started.send(()).unwrap();
1830+
self.release
1831+
.lock()
1832+
.unwrap()
1833+
.recv_timeout(Duration::from_secs(10))
1834+
.expect("test must release blocked discovery");
1835+
}
1836+
Some(PythonEnvironment::new(
1837+
Some(env.executable.clone()),
1838+
Some(PythonEnvironmentKind::Venv),
1839+
env.executable.parent().unwrap().parent().map(PathBuf::from),
1840+
None,
1841+
Some("3.12.0".to_string()),
1842+
))
1843+
}
1844+
1845+
fn find(&self, _reporter: &dyn Reporter) {}
1846+
}
1847+
1848+
fn create_find_test_executable(directory: &std::path::Path) -> PathBuf {
1849+
let executable = if cfg!(windows) {
1850+
directory.join("Scripts").join("python.exe")
1851+
} else {
1852+
directory.join("bin").join("python")
1853+
};
1854+
std::fs::create_dir_all(executable.parent().unwrap()).unwrap();
1855+
std::fs::write(&executable, "").unwrap();
1856+
norm_case(executable)
1857+
}
1858+
1859+
#[test]
1860+
fn find_releases_configuration_lock_and_keeps_directory_snapshot() {
1861+
let workspace = tempfile::tempdir().unwrap();
1862+
let first_executable = create_find_test_executable(&workspace.path().join(".venv"));
1863+
let old_directory = workspace.path().join(".cache");
1864+
let old_executable = create_find_test_executable(&old_directory);
1865+
let new_directory = workspace.path().join(".git");
1866+
create_find_test_executable(&new_directory);
1867+
let (started_tx, started_rx) = mpsc::channel();
1868+
let (release_tx, release_rx) = mpsc::channel();
1869+
let mut context = make_test_context();
1870+
Arc::get_mut(&mut context).unwrap().locators =
1871+
Arc::new(vec![Arc::new(BlockingFindLocator {
1872+
blocked_executable: first_executable.clone(),
1873+
started: started_tx,
1874+
release: Mutex::new(release_rx),
1875+
})]);
1876+
context
1877+
.configuration
1878+
.write()
1879+
.unwrap()
1880+
.config
1881+
.environment_directories = Some(vec![old_directory]);
1882+
1883+
let worker_context = context.clone();
1884+
let options = FindOptions {
1885+
search_path: workspace.path().to_path_buf(),
1886+
};
1887+
let worker = thread::spawn(move || execute_find(&worker_context, &options));
1888+
started_rx.recv_timeout(Duration::from_secs(10)).unwrap();
1889+
let updated = context.configuration.try_write().map(|mut state| {
1890+
state.config.environment_directories = Some(vec![new_directory]);
1891+
state.generation += 1;
1892+
});
1893+
release_tx.send(()).unwrap();
1894+
let environments = worker.join().unwrap();
1895+
1896+
assert!(
1897+
updated.is_ok(),
1898+
"find held the configuration lock during discovery"
1899+
);
1900+
let mut executables: Vec<_> = environments
1901+
.iter()
1902+
.map(|environment| environment.executable.clone().unwrap())
1903+
.collect();
1904+
executables.sort();
1905+
let mut expected = vec![first_executable, old_executable];
1906+
expected.sort();
1907+
assert_eq!(executables, expected);
1908+
assert!(environments.iter().all(|environment| {
1909+
environment.kind == Some(PythonEnvironmentKind::Venv)
1910+
&& environment.version.as_deref() == Some("3.12.0")
1911+
}));
1912+
}
1913+
1914+
#[test]
1915+
fn find_executable_path_preserves_identified_environment() {
1916+
let directory = tempfile::tempdir().unwrap();
1917+
let executable = create_find_test_executable(directory.path());
1918+
let (started_tx, _started_rx) = mpsc::channel();
1919+
let (_release_tx, release_rx) = mpsc::channel();
1920+
let mut context = make_test_context();
1921+
Arc::get_mut(&mut context).unwrap().locators =
1922+
Arc::new(vec![Arc::new(BlockingFindLocator {
1923+
blocked_executable: PathBuf::new(),
1924+
started: started_tx,
1925+
release: Mutex::new(release_rx),
1926+
})]);
1927+
let environments = execute_find(
1928+
&context,
1929+
&FindOptions {
1930+
search_path: executable.clone(),
1931+
},
1932+
);
1933+
assert_eq!(
1934+
environments,
1935+
vec![PythonEnvironment::new(
1936+
Some(executable.clone()),
1937+
Some(PythonEnvironmentKind::Venv),
1938+
executable.parent().unwrap().parent().map(PathBuf::from),
1939+
None,
1940+
Some("3.12.0".to_string()),
1941+
)]
1942+
);
1943+
}
1944+
1945+
#[test]
1946+
fn find_empty_workspace_returns_no_environments() {
1947+
let workspace = tempfile::tempdir().unwrap();
1948+
let context = make_test_context();
1949+
assert!(execute_find(
1950+
&context,
1951+
&FindOptions {
1952+
search_path: workspace.path().to_path_buf()
1953+
},
1954+
)
1955+
.is_empty());
1956+
}
1957+
18081958
#[test]
18091959
fn blocked_refresh_expansion_does_not_block_info() {
18101960
let context = make_test_context();

‎docs/LOCATOR_STATE.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@ Refresh requests run on a transient locator graph. The server configures that gr
44

55
The `Locator::refresh_state()` classification is the contract for that boundary. It keeps configured inputs, self-hydrating caches, and correctness-critical discovery state distinct.
66

7+
## Find Configuration Lock Scope
8+
9+
A directory `find` request copies `environmentDirectories` into an owned local snapshot before workspace discovery. The configuration read lock is released before filesystem traversal or locator identification, so a slow find does not keep configuration publication waiting on that lock. The active search continues using its captured directory list if configuration changes while discovery is running. Executable-only find requests do not read this list.
10+
11+
This is a lock-scope guarantee, not full locator snapshot isolation: find and resolve still use the shared locator graph. Coherent configuration across that graph remains tracked in [#536](https://github.com/microsoft/python-environment-tools/issues/536).
12+
713
## Classifications
814

915
| Classification | Meaning | Sync behavior |

0 commit comments

Comments
 (0)