-
Notifications
You must be signed in to change notification settings - Fork 101
Replace file locking with atomic file operations #1238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This seems like an improvement for launching Julia, but if two |
I'm ok with taking a lock file for write operations to ensure sequential ordering. Although I also think it's probably rare in practice to be an issue. Might as well serialize though. |
That said, I don't see a good reason to use a separate lock file in conjuction with posix advisory locks here. We should either be using pid files like julia precompilation does, or just use fcntl advisory locks (regular advisory locks on windows) on the file itself. |
This commit combines atomic file operations with exclusive file locking to ensure safe concurrent access to configuration files. Changes: - Keep atomic file operations (write to temp file, then rename) - Add exclusive locking on config file during write operations - Lock is acquired in load_mut_config_db before reading - Lock is held throughout the read-modify-write cycle - Lock is automatically released when JuliaupConfigFile is dropped - Removes separate lock file mechanism The approach provides two layers of safety: 1. Exclusive locking ensures write operations are sequenced 2. Atomic rename ensures readers never see partial writes This eliminates race conditions while keeping the code simple. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
f56adc9
to
e88083c
Compare
I've asked Claude to update this to lock on the config file itself. |
Alright, that doesn't quite work that way on windows. I thought it would be quick, but it's not, so I'll need to revisit later when I have more time. |
I'm fairly positive that things will break if we move to a last-write-wins strategy. The config file is not just updated by explicit commands like Having said that, it would still be beneficial to move to an atomic file update strategy (and retain the locking story) simply to deal with things like unexpected process termination in the middle of the write etc. |
I'm trying to remember why I used this separate file there... I think there was some cross-platform twist where the code would have had to be different on Windows vs the rest if we took the lock on the config file itself, and then it was easier to just go with this separate file story? But I don't really remember, it might also have been a different reason. I did have an implementation originally that locked the config file itself and then ran into some kind of problem with that... |
@Keno Could you add tests to this? For file locking you can have a few threads run juliaup operations in parallel, and verify that without locking (or atomics) it gets corrupted, but with locking/atomics, it is not. I do something like this to test rip2's file lock, in case you want to borrow stuff: https://github.com/MilesCranmer/rip2/blob/8bf228f97e069c17b5f24cd4181b36060f637549/tests/integration_tests.rs#L1094-L1181 |
As I proposed in #561 (comment). Unless I'm missing something, this should fix #561. Written by Claude.
This commit removes the lock file mechanism and replaces it with atomic file operations using temporary files for safer concurrent access.
Changes:
The new approach writes configuration changes to a temporary file in the same directory, then atomically renames it to replace the original file. This ensures readers always see either the complete old version or complete new version, never partial writes.
🤖 Generated with Claude Code