Skip to content
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

[Medium] Extend Locks through entire install process #796

Merged
merged 3 commits into from
Aug 24, 2020

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Aug 12, 2020

Info

  • With the implementation of [Medium] Prevent concurrent writes to VOLTA_HOME #684, we are locking the Volta directory while fetching new versions of tools.
  • However, we are not maintaining the lock while performing additional actions (such as writing the default platform file or installing package dependencies).
  • We want to make sure that the lock is maintained throughout the volta install process, while also ensuring that we don't get deadlocks from the ensure_fetched lock (which is needed because it can be called separately by the shims).
  • Ultimately, the goal of VoltaLock is to be a per-process lock, where we prevent other Volta processes from modifying the Volta directory, but allow the current one to make changes.

Changes

  • Refactored the VoltaLock type to keep a count of active locks and only release the file lock when all of them have been released—essentially a refcount style of locking.
  • Ensured that each install function acquires and keeps a lock through the entire volta install process.
  • Also added a lock to the regenerate_shims_for_dir function, to prevent overlapping shim modifications.
  • Added a debug statement to the Drop impl to show when the lock is dropped.

Tested

  • Confirmed through debug statements that the lock is maintained throughout the entire install process for each tool.
  • Confirmed that package installs that require a Node download don't deadlock.

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I wouldn't be sad if there were further comments explaining the why of the use of the count mechanism for future readers of this code, though—it definitely took me a fair bit to work through the relationship between processes, count, and the mutex. I think that's partly because there are two notions of locking here: locking the mutex, and acquiring a VoltaLock.

@charlespierce
Copy link
Contributor Author

Will do, thanks!

@charlespierce charlespierce merged commit 350f02c into volta-cli:main Aug 24, 2020
@charlespierce charlespierce deleted the concurrent_install_2 branch August 24, 2020 23:31
@chriskrycho
Copy link
Contributor

Those explanatory notes in the code are fabulous. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants