-
Notifications
You must be signed in to change notification settings - Fork 279
Advisory locking for metadata and artifacts files #2861
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
Draft
jku
wants to merge
10
commits into
develop
Choose a base branch
from
advisory-locking
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This likely fails on all platforms right now, but the Windows behaviour cannot be fixed without actual locking. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Use get_targetinfo() so that the delegated role loading is tested as well Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
This should prevent issues with multiple processes trying to write at same time. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Otherwise another process might delete the file underneath us Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
There does not seem to be a way around a ugly loop over open()... Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
The file locking should make multiple processes safe Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Will take a look next week
Jussi Kukkonen ***@***.***> schrieb am Fr. 29. Aug. 2025 um
10:05:
… @jku <https://github.com/jku> requested review from
@theupdateframework/python-tuf-maintainers on: #2861
<#2861> Advisory
locking for metadata and artifacts files as a code owner.
—
Reply to this email directly, view it on GitHub
<#2861 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEP4DGGY4A3AFPOCIZH46L3QACWJAVCNFSM6AAAAACFDY6DBSVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJZGQYDMNRUGQ4TQNA>
.
You are receiving this because your review was requested.Message ID:
***@***.***
com>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add advisory file locking to make it safer to run multiple updaters (using the same metadata directory) in separate processes. This should help with #2836
<METADATA_DIR>/.lock
: this lock is used inUpdater.__init__()
,Updater.refresh()
andUpdater.get_targetinfo()
-- lock is typically held through the whole method (in other words while the metadata is being downloaded as well)@stefanberger comments welcome
Implementation notes
The implementation has just enough platform specific complexity that I'm not totally happy with it. There's clearly a chance of bugs here... Unfortunately I'm not convinced using a dependency would make that possibility go away
mscvrt.locking()
is the recommended API but 99% of the time it's not useful because we need a file handle to call it, and we can't get one because another process already has the file open. So we need a dumb loop that keeps callingopen()
andsleep()
. It's ugly but no-one seems to have a better solutionfcntl.lockf()
until the lock opens. Both have a tradeoff: On windows the updater might fail if another updater is a bit slow. On posix, if one updater hangs with a lock for some reason, all future updaters will just wait indefinitely until the original hung process is killedTODO
Alternatives
https://github.com/tox-dev/filelock/ looks reasonable -- although artifact locking will be either more complicated or less parallel since you can't use filelock on the actual file you want to write.