Don't hold the configuration lock during downloads - #1517
Conversation
f5bc01c to
c1827a3
Compare
davidanthoff
left a comment
There was a problem hiding this comment.
It all looks great to me!
I did not try to run it yet, but maybe we merge and push out something on the dev channel and see how it goes?
|
Great. I think we should get #1525 in and test this branch on it before merge. |
Shrink the region covered by the global juliaup configuration lock so that
the network download/extract of a new version no longer happens while the
exclusive lock is held. `juliaup add` now:
1. takes the shared lock only briefly to check whether the channel is
already installed, then releases it;
2. downloads and extracts the version into a temporary directory inside
juliauphome with NO lock held;
3. re-acquires the exclusive lock only to atomically rename the download
into place and update the config.
This splits the old `install_version` into `download_version_to_temp`
(lock-free, network-bound) and `commit_version_install` (lock-held, fast
rename + config write). `install_version` is retained as a thin wrapper for
existing callers.
How this honors the discussion (#435, #561):
- Implements davidanthoff's proposed design from #435: read config + release
lock, download, then re-acquire and commit, instead of holding the write
lock for the whole download.
- Honors LilithHafner's refinement: the commit phase re-checks whether the
install is still needed, so two processes installing the same version in
parallel resolve cleanly (the loser discards its temp dir) rather than
erroring.
- Keeps StefanKarpinski's shared/exclusive model: many readers (e.g. the
launcher checking for self-update / auto-install) coexist and only block
for the millisecond-scale commit, not for a multi-minute download.
Benefit over the other open PRs:
- vs #1238 (remove file locking entirely, atomic-rename only): juliaup
performs multi-file operations (install dir + config + symlinks) that a
single-file atomic rename cannot keep mutually consistent. This change
retains the lock for cross-file consistency while still eliminating the
long-lock pathology that #1238 targets, addressing davidanthoff's stated
objection to dropping the lock outright.
- vs #1317 (close config files right after read): complementary rather than
competing — that PR shortens how long handles stay open; this PR shortens
how long the exclusive lock is held during the expensive download. They
compose cleanly.
Mirror the add command: read the configured juliaup channel under a short-lived shared lock, release it, then perform all network operations (version check + binary download/extract) with no lock held. Re-acquire the exclusive lock only briefly to record the self-update timestamp. Previously `juliaup self update` held the exclusive lock across the network downloads, which blocked concurrent julia/juliaup invocations (they only need a shared read lock) and surfaced as spurious "Juliaup configuration is locked by another process" stalls (#1524). The launcher spawns self update as a background daemon, so a slow download could stall an unrelated second `julia` invocation. This also removes the drop(config_file) added in #1515, which is no longer needed since the lock is released long before the post-update hook runs. Fixes #1524
Refactor `juliaup update` into two phases so the network download no longer happens while the exclusive configuration lock is held: 1. Snapshot the configuration under a short-lived shared lock, release it, then download all needed versions (lock-free) via prepare_channel_update. 2. Re-acquire the exclusive lock only to commit the prepared updates via commit_channel_update (rename + config write + symlinks). The commit phase re-checks that each channel still exists before applying, so a channel removed concurrently is skipped rather than erroring. System-channel updates reuse an already-installed target version without downloading. This removes the last long-lock-across-network pathology in the launcher's background daemon path (versiondb, add, self update, and now update), addressing the spurious "configuration is locked by another process" stalls in #1524. The now-unused install_version wrapper is removed; its callers go through download_version_to_temp + commit_version_install directly.
The "Juliaup configuration is locked by another process, waiting for it to unlock." message was printed immediately whenever the lock could not be acquired on the first try, even though the contending holder usually releases it within milliseconds (e.g. while committing a config change). Add a lock_with_delayed_message helper that polls try_lock for up to 1 second before printing the message and falling back to a blocking wait_lock. Both the shared lock (get_read_lock) and exclusive lock (load_mut_config_db) now route through it, so the message only appears when the lock genuinely stays held longer than the grace period.
Document the shared/exclusive lock model, the "never hold the exclusive lock across a network operation" rule, the 1s grace period before the lock-wait message, and mermaid flow charts for julialauncher, update_version_db, add, self update, and update, plus the #1524 scenario.
c1827a3 to
9da3103
Compare
|
Nice work on this @IanButterworth , it seems well thought-out It does seem to leave Do you think there's a chance to tighten those permissions so that read-only filesystems work? |
|
@topolarity see #1532. |
|
I guess the documentation URL should be: https://github.com/JuliaLang/juliaup/blob/main/devdocs/locking_and_update_flows.md Perhaps not primarily targeted at human eyes though. |
|
Those are dev docs not user docs. |
Closes #435
Closes #561
Closes #1524
Existing draft PRs
Closes #1238
Closes #1317
Closes #746
Claude:
Shrink the region covered by the global juliaup configuration lock so the network download/extract of a version no longer happens while the exclusive lock is held. The same pattern is now applied to every command that downloads:
add— splits the oldinstall_versionintodownload_version_to_temp(lock-free, network-bound) andcommit_version_install(lock-held, fast rename + config write).self update— reads the configured channel under a brief shared lock, releases it, downloads juliaup with no lock held, then re-acquires the exclusive lock only to record the timestamp.update— two-phase: snapshot the config under a shared lock, download all needed channels lock-free (prepare_channel_update), then take the exclusive lock once to commit them all (commit_channel_update).update_version_dbalready followed this pattern.In every case the commit phase re-checks the config (optimistic concurrency), so two processes acting on the same version/channel resolve cleanly (the loser discards its temp dir) rather than erroring.
Also:
Together these eliminate the long-lock-across-network pathology in the launcher's background daemon path (it spawns
self update/ versiondb update), which was the cause of the spurious lock stalls in #1524.How this honors the discussion (#435, #561):
Benefit over the other open PRs: