From 5051b2c0048eaf0c0951da416d948a17be01d6e8 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 16 Jan 2024 22:07:39 +0100 Subject: [PATCH] Use tempfile to prevent install io race crashes (#929) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On ubuntu and python 3.10, ``` cargo run -q -- pip-install --find-links https://storage.googleapis.com/jax-releases/jax_cuda_releases.html "jax[cuda12_pip]==0.4.23" ``` non-deterministically but for me consistently fails to install with messages such as ``` error: Failed to install: nvidia_nccl_cu12-2.19.3-py3-none-manylinux1_x86_64.whl (nvidia-nccl-cu12==2.19.3) Caused by: failed to remove file `/home/konsti/projects/puffin/.venv/lib/python3.10/site-packages/nvidia/__init__.py` Caused by: No such file or directory (os error 2) ``` ``` error: Failed to install: nvidia_cublas_cu12-12.3.4.1-py3-none-manylinux1_x86_64.whl (nvidia-cublas-cu12==12.3.4.1) Caused by: Replacing an existing file or directory failed ``` ``` error: Failed to install: nvidia_cuda_nvcc_cu12-12.3.107-py3-none-manylinux1_x86_64.whl (nvidia-cuda-nvcc-cu12==12.3.107) Caused by: failed to hardlink file from /home/konsti/.cache/puffin/wheels-v0/pypi/nvidia-cuda-nvcc-cu12/nvidia_cuda_nvcc_cu12-12.3.107-py3-none-manylinux1_x86_64/nvidia/__init__.py to /home/konsti/projects/puffin/.venv/lib/python3.10/site-packages/nvidia/__init__.py Caused by: File exists (os error 17) ``` We install a lot of nvidia package, that all contain `nvidia/__init__.py`, since they all install themselves into the `nvidia` module: ``` nvidia-cublas-cu12==12.3.4.1 nvidia-cuda-cupti-cu12==12.3.101 nvidia-cuda-nvcc-cu12==12.3.107 nvidia-cuda-nvrtc-cu12==12.3.107 nvidia-cuda-runtime-cu12==12.3.101 nvidia-cudnn-cu12==8.9.7.29 nvidia-cufft-cu12==11.0.12.1 nvidia-cusolver-cu12==11.5.4.101 nvidia-cusparse-cu12==12.2.0.103 nvidia-nccl-cu12==2.19.3 nvidia-nvjitlink-cu12==12.3.101 ``` ``` $ tree -L 1 .venv/lib/python3.10/site-packages/nvidia .venv/lib/python3.10/site-packages/nvidia ├── cublas ├── cuda_cupti ├── cuda_nvcc ├── cuda_nvrtc ├── cuda_runtime ├── cudnn ├── cufft ├── cusolver ├── cusparse ├── __init__.py ├── nccl └── nvjitlink ``` When installing we get a race condition, each package installation is its own thread: * Installer Thread 1 creates `nvidia/__init__.py` * Installer Thread 2 sees an existing `nvidia/__init__.py` * Installer Thread 3 sees an existing `nvidia/__init__.py` * Installer Thread 2 removes `nvidia/__init__.py` * Installer Thread 3 tries to remove `nvidia/__init__.py`, it doesn't exist anymore -> failure. We switch to a new strategy: When the target files exists, we don't remove it, but instead hardlink the source file to a tempfile first, then renaming the tempfile to the target file. Renaming is considered an atomic operation. I've put the logging on debug level because they cases indicate a conflict between two packages while being rare. Closes #925 --------- Co-authored-by: Charlie Marsh --- crates/install-wheel-rs/src/linker.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/crates/install-wheel-rs/src/linker.rs b/crates/install-wheel-rs/src/linker.rs index ec016684e..d7f70e845 100644 --- a/crates/install-wheel-rs/src/linker.rs +++ b/crates/install-wheel-rs/src/linker.rs @@ -6,6 +6,7 @@ use std::path::Path; use configparser::ini::Ini; use fs_err as fs; use fs_err::File; +use tempfile::tempdir_in; use tracing::{debug, instrument}; use pypi_types::DirectUrl; @@ -398,11 +399,18 @@ fn hardlink_wheel_files( if let Err(err) = fs::hard_link(path, &out_path) { // If the file already exists, remove it and try again. if err.kind() == std::io::ErrorKind::AlreadyExists { - fs::remove_file(&out_path)?; - if fs::hard_link(path, &out_path).is_err() { + debug!( + "File already exists (initial attempt), overwriting: {}", + out_path.display() + ); + // Removing and recreating would lead to race conditions. + let tempdir = tempdir_in(&site_packages)?; + let tempfile = tempdir.path().join(entry.file_name()); + if fs::hard_link(path, &tempfile).is_err() { fs::copy(path, &out_path)?; attempt = Attempt::UseCopyFallback; } + fs_err::rename(&tempfile, &out_path)?; } else { fs::copy(path, &out_path)?; attempt = Attempt::UseCopyFallback; @@ -413,8 +421,15 @@ fn hardlink_wheel_files( if let Err(err) = fs::hard_link(path, &out_path) { // If the file already exists, remove it and try again. if err.kind() == std::io::ErrorKind::AlreadyExists { - fs::remove_file(&out_path)?; - fs::hard_link(path, &out_path)?; + debug!( + "File already exists (subsequent attempt), overwriting: {}", + out_path.display() + ); + // Removing and recreating would lead to race conditions. + let tempdir = tempdir_in(&site_packages)?; + let tempfile = tempdir.path().join(entry.file_name()); + fs::hard_link(path, &tempfile)?; + fs_err::rename(&tempfile, &out_path)?; } else { return Err(err.into()); }