Fix multi-user file locking #10265
Fix multi-user file locking #10265
Conversation
| #[allow(unsafe_code)] | ||
| unsafe { | ||
| nix::libc::umask(old_umask); | ||
| }; |
There was a problem hiding this comment.
Can you use nix::sys::stat::umask here instead? (Otherwise, you're adding a dependency here on nix when just libc would do. But if we're depending on nix, we might as well use the safe API...)
There was a problem hiding this comment.
Can you use
nix::sys::stat::umaskhere instead? (Otherwise, you're adding a dependency here onnixwhen justlibcwould do. But if we're depending onnix, we might as well use the safe API...)
Tested, nix::sys::stat::umask works and is safe.
| let file = fs_err::File::create(path.as_ref())?; | ||
| #[allow(unsafe_code)] | ||
| #[cfg(unix)] | ||
| let old_umask = unsafe { nix::libc::umask(0) }; // for multi user file locking |
There was a problem hiding this comment.
I unfortunately think this is not quite right. umask is a process global property, but we might have multiple threads in the same process creating files. So when you twiddle the umask like this, you might very well be impacting the creation of other completely unrelated files in an undesirable way.
Is it possible to change the permissions after the file is created instead?
I suppose another alternative here is to suggest that end users change their umask when running uv if they want it to run in multi-user mode.
There was a problem hiding this comment.
Thanks for the review.
Is it possible to change the permissions after the file is created instead?
Yes, but multiple processes can cause errors between the time of creation and the time of changing permissions.
I suppose another alternative here is to suggest that end users change their umask when running uv if they want it to run in multi-user mode.
Another alternative I have is to set the umask at main.
Yet another alternative to change the locking implementation to check the existence of a .lock file. Then, the .lock file must be deleted when unlocking.
If a uv process abnormally terminates without unlocking, subsequent runs may cause a deadlock. If that happens the Python environment is possibly broken and needs to be set up again anyway.
There was a problem hiding this comment.
I think we need to be very cautious about changing our locking strategy. The strategy you're proposing is the "classic" approach (although for maximum portability, a directory and not a file should be used, as SQLite does IIRC), but it has the predictable downside as you point out: on some occasions, the lock file isn't deleted when unlocked. And the only way around this is user intervention, which is usually to delete the lock file and "hope for the best." Probably in this context, the simplest approach would be for the user to just completely delete the virtual environment.
IMO, the success of this strategy depends a lot on how often that lock file hangs around. In a perfect world, sure, its existence after process termination implies something went wrong with uv and thus the user should be blowing away the venv anyway. But I'm not convinced that this is the only way for a lock file to hang around. It's not uncommon for me to use programs that utilize this locking strategy, and despite their best efforts, I still need to remove the lock file from time to time.
Have you tried setting umask yourself outside of uv? Does that work? If it does, then I kinda feel like that's the most appropriate solution here. Because you're fundamentally working in a shared environment, and it seems sensible to set a umask that supports that environment.
There was a problem hiding this comment.
I think we need to be very cautious about changing our locking strategy. The strategy you're proposing is the "classic" approach (although for maximum portability, a directory and not a file should be used, as SQLite does IIRC), but it has the predictable downside as you point out: on some occasions, the lock file isn't deleted when unlocked. And the only way around this is user intervention, which is usually to delete the lock file and "hope for the best." Probably in this context, the simplest approach would be for the user to just completely delete the virtual environment.
IMO, the success of this strategy depends a lot on how often that lock file hangs around. In a perfect world, sure, its existence after process termination implies something went wrong with uv and thus the user should be blowing away the venv anyway. But I'm not convinced that this is the only way for a lock file to hang around. It's not uncommon for me to use programs that utilize this locking strategy, and despite their best efforts, I still need to remove the lock file from time to time.
Thanks for the explanation.
Have you tried setting
umaskyourself outside of uv? Does that work? If it does, then I kinda feel like that's the most appropriate solution here. Because you're fundamentally working in a shared environment, and it seems sensible to set aumaskthat supports that environment.
Yes, external setting of umask does work and also pointed out in #8032 (comment).
There was a problem hiding this comment.
I think this PR can be closed then.
Summary
Try to resolve #8032 with suggestion in #8032 (comment)
Test Plan
Tested on the Dockerfile (minimal reproducible example) in #8032 (comment)