canonical-ubuntu-qa team mailing list archive

[Skia <mp+470495@xxxxxxxxxxxxxxxxxx>]

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.