mirror of
https://github.com/tree-sitter/tree-sitter.git
synced 2026-09-10 07:26:23 -04:00
fix(loader): replace flock-based compilation lock with atomic lock file
The previous two-phase locking scheme (probe existing lock file, then create and compile) had TOCTOU races between phases that caused spurious failures in CI when tests compiled grammars concurrently. Replace with a single-phase approach using `create_new` as the sole synchronization primitive. An RAII `LockFile` guard ensures cleanup on drop (including panics). Only "builders" attempt to acquire the lock. Loaders simply load the file. Builders compile a temporary path and then rename, so loaders are guaranteed a valid shared library. The winning builder compiles and then drops the lock. Losers poll for lock file removal, then load. Stale locks from killed processes are detected via a timeout and an appropriate error message is displayed to the user.
This commit is contained in:
Will Lillis
parent
3294a64027
commit
f303a957a3
|
|
@ -15,7 +15,7 @@ use std::{
|
|||
path::{Path, PathBuf},
|
||||
process::Command,
|
||||
sync::Mutex,
|
||||
time::{SystemTime, SystemTimeError},
|
||||
time::{Duration, Instant, SystemTime},
|
||||
};
|
||||
|
||||
use etcetera::BaseStrategy as _;
|
||||
|
|
@ -84,6 +84,10 @@ pub enum LoaderError {
|
|||
Compiler(CompilerError),
|
||||
#[error("Parser compilation failed.\nStdout: {0}\nStderr: {1}")]
|
||||
Compilation(String, String),
|
||||
#[error(
|
||||
"Lock file '{0}' appears stale\nIf there isn't another concurrent tree-sitter instance, remove it"
|
||||
)]
|
||||
LockFileTimeout(PathBuf),
|
||||
#[error("Failed to execute curl for {0} -- {1}")]
|
||||
Curl(String, std::io::Error),
|
||||
#[error("Failed to load language in current directory:\n{0}")]
|
||||
|
|
@ -118,8 +122,6 @@ pub enum LoaderError {
|
|||
Tags(#[from] TagsError),
|
||||
#[error("Failed to execute tar for {0} -- {1}")]
|
||||
Tar(String, std::io::Error),
|
||||
#[error(transparent)]
|
||||
Time(#[from] SystemTimeError),
|
||||
#[error("Unknown scope '{0}'")]
|
||||
UnknownScope(String),
|
||||
#[error("Failed to download {tool} from {url}")]
|
||||
|
|
@ -711,6 +713,74 @@ impl<'a> CompileConfig<'a> {
|
|||
|
||||
unsafe impl Sync for Loader {}
|
||||
|
||||
/// Generate a temporary file path for atomic writes. The temp file is a hidden
|
||||
/// dotfile alongside the target, tagged with the current process and thread id
|
||||
/// for debuggability (e.g. `.javascript.so.12345.ThreadId(3)`).
|
||||
fn temp_path(path: &Path) -> PathBuf {
|
||||
let filename = path
|
||||
.file_name()
|
||||
.expect("output_path must have a filename")
|
||||
.to_string_lossy();
|
||||
path.with_file_name(format!(
|
||||
".{filename}.{}.{:?}",
|
||||
std::process::id(),
|
||||
std::thread::current().id()
|
||||
))
|
||||
}
|
||||
|
||||
/// RAII lock file guard. The lock file is created atomically via
|
||||
/// [`create_new`](`fs::OpenOptions::create_new`) and removed on drop.
|
||||
/// and removed on drop.
|
||||
struct LockFile {
|
||||
path: PathBuf,
|
||||
}
|
||||
|
||||
impl LockFile {
|
||||
/// Attempt to atomically create the lock file.
|
||||
///
|
||||
/// Returns `Ok(Some)` if we won the race, `Ok(None)` if we lost,
|
||||
/// or the underlying IO error otherwise.
|
||||
fn create(path: &Path) -> LoaderResult<Option<Self>> {
|
||||
match fs::OpenOptions::new()
|
||||
.create_new(true)
|
||||
.write(true)
|
||||
.open(path)
|
||||
{
|
||||
Ok(_) => Ok(Some(Self {
|
||||
path: path.to_path_buf(),
|
||||
})),
|
||||
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => Ok(None),
|
||||
Err(e) => Err(LoaderError::IO(IoError::new(e, Some(path)))),
|
||||
}
|
||||
}
|
||||
|
||||
/// Wait for an existing lock file to be removed by whoever created it.
|
||||
/// If the lock file persists beyond `timeout`, return [`LoaderError::LockFileTimeout`]
|
||||
fn wait_for_removal(path: &Path, timeout: Duration) -> LoaderResult<()> {
|
||||
let mut sleep_ms = 100;
|
||||
let deadline = Instant::now() + timeout;
|
||||
while path.exists() {
|
||||
if Instant::now() > deadline {
|
||||
return Err(LoaderError::LockFileTimeout(path.to_path_buf()));
|
||||
}
|
||||
std::thread::sleep(Duration::from_millis(sleep_ms));
|
||||
sleep_ms = (sleep_ms * 2).min(1000);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for LockFile {
|
||||
fn drop(&mut self) {
|
||||
match fs::remove_file(&self.path) {
|
||||
Ok(()) => {}
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
|
||||
Err(e) => warn!("Failed to remove lock file '{}': {e}.", self.path.display()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl Loader {
|
||||
pub fn new() -> LoaderResult<Self> {
|
||||
let parser_lib_path = if let Ok(path) = env::var("TREE_SITTER_LIBDIR") {
|
||||
|
|
@ -1063,25 +1133,6 @@ impl Loader {
|
|||
recompile = needs_recompile(&output_path, &paths_to_check)?;
|
||||
}
|
||||
|
||||
#[cfg(feature = "wasm")]
|
||||
if let Some(wasm_store) = self.wasm_store.lock().unwrap().as_mut() {
|
||||
if recompile {
|
||||
self.compile_parser_to_wasm(
|
||||
&config.name,
|
||||
config.src_path,
|
||||
config
|
||||
.scanner_path
|
||||
.as_ref()
|
||||
.and_then(|p| p.strip_prefix(config.src_path).ok()),
|
||||
&output_path,
|
||||
)?;
|
||||
}
|
||||
|
||||
let wasm_bytes = fs::read(&output_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(output_path.as_path()))))?;
|
||||
return Ok(wasm_store.load_language(&config.name, &wasm_bytes)?);
|
||||
}
|
||||
|
||||
// Create a unique lock path based on the output path hash to prevent
|
||||
// interference when multiple processes build the same grammar (by name)
|
||||
// to different output locations
|
||||
|
|
@ -1091,85 +1142,79 @@ impl Loader {
|
|||
format!("{:x}", hasher.finish())
|
||||
};
|
||||
|
||||
let lock_path = if env::var("CROSS_RUNNER").is_ok() {
|
||||
let mut lock_path = if env::var("CROSS_RUNNER").is_ok() {
|
||||
tempfile::tempdir()
|
||||
.expect("create a temp dir")
|
||||
.path()
|
||||
.to_path_buf()
|
||||
} else {
|
||||
etcetera::choose_base_strategy()?.cache_dir()
|
||||
}
|
||||
.join("tree-sitter")
|
||||
.join("lock")
|
||||
.join(format!("{}-{lock_hash}.lock", config.name));
|
||||
|
||||
if let Ok(lock_file) = fs::OpenOptions::new().write(true).open(&lock_path) {
|
||||
recompile = false;
|
||||
if lock_file.try_lock().is_err() {
|
||||
// if we can't acquire the lock, another process is compiling the parser, wait for
|
||||
// it and don't recompile
|
||||
lock_file
|
||||
.lock()
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?;
|
||||
recompile = false;
|
||||
} else {
|
||||
// if we can acquire the lock, check if the lock file is older than 30 seconds, a
|
||||
// run that was interrupted and left the lock file behind should not block
|
||||
// subsequent runs
|
||||
let time = lock_file
|
||||
.metadata()
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?
|
||||
.modified()
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?
|
||||
.elapsed()?
|
||||
.as_secs();
|
||||
if time > 30 {
|
||||
fs::remove_file(&lock_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?;
|
||||
recompile = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
lock_path.push(format!(
|
||||
"tree-sitter{slash}lock{slash}{name}-{lock_hash}.lock",
|
||||
slash = std::path::MAIN_SEPARATOR,
|
||||
name = config.name
|
||||
));
|
||||
|
||||
// Synchronize compilation across threads/processes using an atomic lock
|
||||
// file. Exactly one caller wins the race to compile. Losers wait for the
|
||||
// lock file to be removed, then skip compilation and proceed to loading.
|
||||
//
|
||||
// Loading only (`recompile == false`) doesn't need a lock because
|
||||
// `compile_parser_to_dylib` writes to a temp file and atomically renames
|
||||
// into place, so loaders always see a complete shared library (either the
|
||||
// new or previous copy).
|
||||
//
|
||||
// The `LockFile` ensures cleanup on drop, and stale locks from killed
|
||||
// processes are detected via a timeout in `wait_for_removal`.
|
||||
if recompile {
|
||||
let parent_path = lock_path.parent().unwrap();
|
||||
fs::create_dir_all(parent_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(parent_path))))?;
|
||||
let lock_file = fs::OpenOptions::new()
|
||||
.create(true)
|
||||
.truncate(true)
|
||||
.write(true)
|
||||
.open(&lock_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?;
|
||||
lock_file
|
||||
.lock()
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path.as_path()))))?;
|
||||
|
||||
self.compile_parser_to_dylib(&config, &lock_file, &lock_path)?;
|
||||
|
||||
if config.scanner_path.is_some() {
|
||||
Self::check_external_scanner(&output_path);
|
||||
match LockFile::create(&lock_path)? {
|
||||
Some(_lock) => {
|
||||
// We won the race, so compile with the lock.
|
||||
let compile_wasm;
|
||||
#[cfg(feature = "wasm")]
|
||||
{
|
||||
compile_wasm = self.wasm_store.lock().unwrap().is_some();
|
||||
}
|
||||
#[cfg(not(feature = "wasm"))]
|
||||
{
|
||||
compile_wasm = false;
|
||||
};
|
||||
#[cfg(feature = "wasm")]
|
||||
if compile_wasm {
|
||||
self.compile_parser_to_wasm(
|
||||
&config.name,
|
||||
config.src_path,
|
||||
config
|
||||
.scanner_path
|
||||
.as_ref()
|
||||
.and_then(|p| p.strip_prefix(config.src_path).ok()),
|
||||
&output_path,
|
||||
)?;
|
||||
}
|
||||
if !compile_wasm {
|
||||
self.compile_parser_to_dylib(&config)?;
|
||||
if config.scanner_path.is_some() {
|
||||
Self::check_external_scanner(&output_path);
|
||||
}
|
||||
}
|
||||
// _lock dropped here, removing the lock file.
|
||||
}
|
||||
// Another thread/process is compiling (or a previous run
|
||||
// crashed and left a stale lock). Wait for it to finish.
|
||||
None => LockFile::wait_for_removal(&lock_path, Duration::from_secs(30))?,
|
||||
}
|
||||
}
|
||||
|
||||
// Ensure the dynamic library exists before trying to load it. This can
|
||||
// happen in race conditions where we couldn't acquire the lock because
|
||||
// another process was compiling but it still hasn't finished by the
|
||||
// time we reach this point, so the output file still doesn't exist.
|
||||
//
|
||||
// Instead of allowing the `load_language` call below to fail, return a
|
||||
// clearer error to the user here.
|
||||
if !output_path.exists() {
|
||||
let msg = format!(
|
||||
"Dynamic library `{}` not found after build attempt. \
|
||||
Are you running multiple processes building to the same output location?",
|
||||
output_path.display()
|
||||
);
|
||||
|
||||
Err(LoaderError::IO(IoError::new(
|
||||
std::io::Error::new(std::io::ErrorKind::NotFound, msg),
|
||||
Some(output_path.as_path()),
|
||||
)))?;
|
||||
#[cfg(feature = "wasm")]
|
||||
if let Some(wasm_store) = self.wasm_store.lock().unwrap().as_mut() {
|
||||
let wasm_bytes = fs::read(&output_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(output_path.as_path()))))?;
|
||||
return Ok(wasm_store.load_language(&config.name, &wasm_bytes)?);
|
||||
}
|
||||
|
||||
Self::load_language(&output_path, &language_fn_name)
|
||||
|
|
@ -1198,12 +1243,7 @@ impl Loader {
|
|||
Ok(language)
|
||||
}
|
||||
|
||||
fn compile_parser_to_dylib(
|
||||
&self,
|
||||
config: &CompileConfig,
|
||||
lock_file: &fs::File,
|
||||
lock_path: &Path,
|
||||
) -> LoaderResult<()> {
|
||||
fn compile_parser_to_dylib(&self, config: &CompileConfig) -> LoaderResult<()> {
|
||||
let mut cc_config = cc::Build::new();
|
||||
cc_config
|
||||
.cargo_metadata(false)
|
||||
|
|
@ -1237,8 +1277,12 @@ impl Loader {
|
|||
|
||||
let output_path = config.output_path.as_ref().unwrap();
|
||||
|
||||
// Compile to a temporary path, then atomically rename into place.
|
||||
// This ensures loaders never see a half-written .so file.
|
||||
let temp_output = temp_path(output_path);
|
||||
|
||||
let temp_dir = if compiler.is_like_msvc() {
|
||||
let out = format!("-out:{}", output_path.to_str().unwrap());
|
||||
let out = format!("-out:{}", temp_output.to_str().unwrap());
|
||||
command.arg(if self.debug_build { "-LDd" } else { "-LD" });
|
||||
command.arg("-utf-8");
|
||||
|
||||
|
|
@ -1271,7 +1315,7 @@ impl Loader {
|
|||
command.arg("-lc");
|
||||
}
|
||||
command.args(cc_config.get_files());
|
||||
command.arg("-o").arg(output_path);
|
||||
command.arg("-o").arg(&temp_output);
|
||||
|
||||
None
|
||||
};
|
||||
|
|
@ -1300,15 +1344,14 @@ impl Loader {
|
|||
let _ = fs::remove_dir_all(temp_dir);
|
||||
}
|
||||
|
||||
lock_file
|
||||
.unlock()
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path))))?;
|
||||
fs::remove_file(lock_path)
|
||||
.map_err(|e| LoaderError::IO(IoError::new(e, Some(lock_path))))?;
|
||||
|
||||
if output.status.success() {
|
||||
fs::rename(&temp_output, output_path).map_err(|e| {
|
||||
let _ = fs::remove_file(&temp_output);
|
||||
LoaderError::IO(IoError::new(e, Some(output_path)))
|
||||
})?;
|
||||
Ok(())
|
||||
} else {
|
||||
let _ = fs::remove_file(&temp_output);
|
||||
Err(LoaderError::Compilation(
|
||||
String::from_utf8_lossy(&output.stdout).to_string(),
|
||||
String::from_utf8_lossy(&output.stderr).to_string(),
|
||||
|
|
@ -1377,13 +1420,16 @@ impl Loader {
|
|||
let wasm_opt_exe = Self::ensure_binaryen_exists()?;
|
||||
drop(tool_lock);
|
||||
|
||||
let output_path = output_path.to_str().unwrap();
|
||||
// Compile to a temporary path, then atomically rename into place.
|
||||
// This ensures loaders never see a half-written .wasm file.
|
||||
let temp_output = temp_path(output_path);
|
||||
let temp_output_str = temp_output.to_str().unwrap();
|
||||
|
||||
let mut compile_command = Command::new(&clang_exe);
|
||||
compile_command.current_dir(src_path).args([
|
||||
"--target=wasm32-unknown-wasi",
|
||||
"-o",
|
||||
output_path,
|
||||
temp_output_str,
|
||||
"-fPIC",
|
||||
"-shared",
|
||||
"--no-wasm-opt",
|
||||
|
|
@ -1420,6 +1466,7 @@ impl Loader {
|
|||
}
|
||||
|
||||
if !compile_output.status.success() {
|
||||
let _ = fs::remove_file(&temp_output);
|
||||
return Err(LoaderError::WasmCompilation(
|
||||
String::from_utf8_lossy(&compile_output.stderr).to_string(),
|
||||
));
|
||||
|
|
@ -1428,7 +1475,7 @@ impl Loader {
|
|||
let mut opt_command = Command::new(&wasm_opt_exe);
|
||||
opt_command
|
||||
.current_dir(src_path)
|
||||
.args([output_path, "-Os", "-o", output_path]);
|
||||
.args([temp_output_str, "-Os", "-o", temp_output_str]);
|
||||
|
||||
if self.verbose {
|
||||
display_build_cmd(&opt_command);
|
||||
|
|
@ -1445,11 +1492,17 @@ impl Loader {
|
|||
}
|
||||
|
||||
if !opt_output.status.success() {
|
||||
let _ = fs::remove_file(&temp_output);
|
||||
return Err(LoaderError::WasmOptimization(
|
||||
String::from_utf8_lossy(&opt_output.stderr).to_string(),
|
||||
));
|
||||
}
|
||||
|
||||
fs::rename(&temp_output, output_path).map_err(|e| {
|
||||
let _ = fs::remove_file(&temp_output);
|
||||
LoaderError::IO(IoError::new(e, Some(output_path)))
|
||||
})?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue