Just for the sake of it, and out of curiosity, I tested to run the cache code
and try to trigger a race.
I added this [1] at the end of `cache.py`, and ran it with: `rm
/dev/shm/test_cache.json; for i in $(seq 5); do python3 ./cache.py &; sleep
0.2; done`
The results were instant: with the previous version that had a rename, it very
quickly fails with a JSONDecodeError, and the cache file get borked for some
reason (probably because every process use the same tmp file). This current
version without the rename works just fine, hurray :-)
Please just fix the two small remaining comments, and this will be good to go.
[1]:
```
def main():
import random
seed = random.randint(0, 1000000)
cache = KVCache("/dev/shm/test_cache.json")
for i in range(1000):
cache.set(f"{seed}-{i}", f"Hello there {i}")
value = cache.get(f"{seed}-{i}")
assert value == f"Hello there {i}"
if __name__ == "__main__":
main()
```
Diff comments:
> diff --git a/charms/focal/autopkgtest-web/webcontrol/helpers/cache.py
> b/charms/focal/autopkgtest-web/webcontrol/helpers/cache.py
> new file mode 100644
> index 0000000..f2c6358
> --- /dev/null
> +++ b/charms/focal/autopkgtest-web/webcontrol/helpers/cache.py
> @@ -0,0 +1,60 @@
> +import fcntl
> +import json
> +from pathlib import Path
> +
> +
> +class KVCache:
nit: KeyValueCache could be even more explicit, especially when people already
deal with insane amount of acronyms nowadays :-)
> + def __init__(self, cache_path):
> + self.path = Path(cache_path)
> + if not self.path.exists():
> + with open(self.path, "w") as f:
> + json.dump({}, f)
> +
> + def _lock(self, file, mode):
> + if mode == "r":
> + fcntl.flock(file, fcntl.LOCK_SH)
> + if mode == "w":
> + fcntl.flock(file, fcntl.LOCK_EX)
> +
> + def _unlock(self, file):
> + fcntl.flock(file, fcntl.LOCK_UN)
> +
> + def _write(self, data):
> + with open(self.path, "r+") as f:
Is it needed to reopen the file here? Is there an argument against just passing
the file descriptor to that function? If no, then that would probably be better.
> + f.seek(0)
> + json.dump(data, f, default=str)
> + f.truncate()
> +
> + def get(self, key):
> + with open(self.path, "r") as f:
> + self._lock(f, "r")
> + try:
> + data = json.load(f)
> + return data.get(key, None)
> + finally:
> + self._unlock(f)
> +
> + def set(self, key, value):
> + with open(self.path, "r+") as f:
> + self._lock(f, "w")
> + try:
> + data = json.load(f)
> + data[key] = value
> + self._write(data)
> + finally:
> + self._unlock(f)
> +
> + def delete(self, key):
> + with open(self.path, "r+") as f:
> + self._lock(f, "w")
> + try:
> + data = json.load(f)
> + if key in data:
> + del data[key]
> + self._write(data)
> + finally:
> + self._unlock(f)
> +
> + def clear(self):
> + with open(self.path, "w") as f:
> + json.dump({}, f)
--
https://code.launchpad.net/~uralt/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/470495
Your team Canonical's Ubuntu QA is requested to review the proposed merge of
~uralt/autopkgtest-cloud:allowed-user-cache into autopkgtest-cloud:master.
--
Mailing list: https://launchpad.net/~canonical-ubuntu-qa
Post to : canonical-ubuntu-qa@lists.launchpad.net
Unsubscribe : https://launchpad.net/~canonical-ubuntu-qa
More help : https://help.launchpad.net/ListHelp
