added basic file locking capabilities for unix and windows#285
added basic file locking capabilities for unix and windows#285jonasmalacofilho merged 31 commits intomasterfrom
Conversation
jonasmalacofilho
force-pushed
the
274-keyval-file-locking
branch
from
d6b0277 to
be5ad1c
Compare
Windows file locking will have an issue where if the program crashes before the lock file gets deleted the program will be blocked
…ded a couple lines back in that accidentally got removed
This reverts commit 409869b ("made change to make it work on the mac pipelines"), as it was broken (missing `assert` keywords effectively disables the assertions) and only introduced unreachable changes (typo in `dawrin` prevents any existing platform to enter those branches). It is also notable that Mac OS and Linux behaviors only deviate in a single test. If the reverted commit had worked as intended, it would have *broken* two other tests.
…updated the hydro platinum driver to match
This makes all tests that depend on runtime storage more robust. Mocking the entire API allowed it to be get out of sync with the real implementation, and is not necessary in this (simple) case.
jonasmalacofilho
force-pushed
the
274-keyval-file-locking
branch
from
e268f21 to
142b993
Compare
jonasmalacofilho
force-pushed
the
274-keyval-file-locking
branch
from
634d32c to
62a8239
Compare
MarshallAsch
left a comment
MarshallAsch
left a comment
There was a problem hiding this comment.
Aside from the comments I don't immediately spot any major issues,
| class MockRuntimeStorage(RuntimeStorage): | ||
| def __init__(self, key_prefixes, backend=None): | ||
| if not backend: | ||
| run_dir = mkdtemp('run_dir') |
There was a problem hiding this comment.
should this use the pytest tempdir utility?
There was a problem hiding this comment.
I would have preferred to use it, yes. But I couldn't figure out how to use the magic tempdir fixture within an arbitrary class. Do you know how it can be done?
There was a problem hiding this comment.
I suppose it could be passed in, but that is a but gross.
liquidctl/keyval.py
Outdated
| def __init__(self, key_prefixes): | ||
| self._backend = _FilesystemBackend(key_prefixes) | ||
| def __init__(self, key_prefixes, backend=None): | ||
| self._backend = backend if backend is not None else _FilesystemBackend(key_prefixes) |
There was a problem hiding this comment.
Self nitpick: this could be written in a more clear way.
Also, should be accept runtime_dirs instead of requiring the tests to construct a private _FilesystemBackend class? This would also avoid having the tests pass key_prefixes both to the backend and to the RuntimeStorage wrapper.
There was a problem hiding this comment.
imo passing a backend to the test could make sense because it would allow us to swap it out with a mock to speed up the driver tests instead of writing to the disk, (which from what my understanding was is best practice anyway?)
There was a problem hiding this comment.
We used to mock the backend without any actual FS access, but in this branch I actually made all tests revert back to a real FS backend, albeit with temporary and isolated locations.
Using a mocked backend requires keeping it in sync with the real implementation, something we weren't great at. And that extra work and risk for a small performance improvement in our test suite is just not worth it, IMO.
2 participants
Closes #274
This should solve issues with multiple instances of liquidctl running at the same time having issues with concurrent access of the data file.
This is being resolved by using lock files to make the functions for the
RuntimeStorageclass atomic.Also adds the function
load_store()which can get a value, update it and then save it atomically.load()'s can happen at the same timeload_storefunction where applicableThere may be an issue on windows that if the program crashes before the lock file can be deleted it will not be able to acquire the lock again, and the program will hang.