-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: lock shared cache directory #1888
Conversation
16a5f34
to
2d9d3df
Compare
Locks the shared cache directory to prevent concurrency issues. Fixes #1845 CRAFT-3313
fs: pyfakefs.fake_filesystem.FakeFilesystem, fake_path, simple_charm | ||
) -> services.CharmcraftServiceFactory: | ||
fake_project_dir = fake_path / "project" | ||
def service_factory(simple_charm, new_path) -> services.CharmcraftServiceFactory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stops using the fake filesystem and uses the real FS. I think it was a mistake for me to use pyfakefs for integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are to deal with the change from using pyfakefs to using a real path.
} | ||
|
||
with provider.instance(**provider_kwargs) as instance: | ||
# Because we've already locked the cache, we shouldn't see the lockfile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused here - why isn't the cache visible? Did the call to _maybe_lock_cache()
inside provider.instance()
fall into the except OSError:
codepath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because we run _maybe_lock_cache
on line 36. Looking back I realise I named the function for the success case and tested for the failure case. I'll fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is surprising to me because I thought flock()
was per-process, so a process trying to flock a file it already holds would be a noop
If this isn't the case, what happens if we do multiple consecutive managed builds? Like a build plan with multiple entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! I wasn't checking for that properly, either. However, it's not quite per-process either. It's per file descriptor, so the same process can't lock the same path in two places if it's got the file open separately twice. (This also requires making two separate Path
objects, as pathlib
will cache the file descriptor for the same object, as I learnt the hard way.)
I've updated it now with a much better lock and test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. If this is an issue that users see frequently then a nicer solution might be more fine-grained cache locking control, so multiple instances of charmcraft could run with the shared cache at once, but they couldn't both use the same package (or whatever the things inside this cache are) at the same time.
5b6a9f3
to
7e134dd
Compare
e8b42bf
to
511ef26
Compare
538fa94
to
325e741
Compare
Locks the shared cache directory to prevent concurrency issues.
Fixes #1845
CRAFT-3313