Add user to cache filename; better handle cache load/save failures #1179
Conversation
|
Thanks, I agree with the changes.
The one thing that bugs me is that now it will always say "Loading grammar from cache ...", even when the cache file doesn't exist. At least, we should change it to "checking for cache for grammar", or something of the sort. @MegaIng Mind also taking a look? |
There was a problem hiding this comment.
@erezsh The behavior of always printing loading from cache might just be only when running tests and not when actually running the code with a real FileSystem. I would think the call to FS.open should already cause the Exception before getting to the logger.debug statement.
lark/lark.py
Outdated
| @@ -308,28 +309,30 @@ def __init__(self, grammar: 'Union[Grammar, str, IO[str]]', **options) -> None: | |||
| if self.options.cache is not True: | |||
| raise ConfigurationError("cache argument must be bool or str") | |||
|
|
|||
| cache_fn = tempfile.gettempdir() + '/.lark_cache_%s_%s_%s.tmp' % (cache_md5, *sys.version_info[:2]) | |||
| cache_fn = tempfile.gettempdir() + "/.lark_cache_%s_%s_%s_%s.tmp" % (getpass.getuser(), cache_md5, *sys.version_info[:2]) | |||
There was a problem hiding this comment.
I am not sure what the gurantees are about the value returned from getpass.getuser(). It could potentially return a string that results in an invalid file name, but we could also wait till someone actually reports a problem with this. In a similar vein, getuser might fail on some weird OSes and raise an exception.
There was a problem hiding this comment.
- Per https://docs.python.org/3/library/getpass.html#getpass.getuser it can raise an exception if attempts at finding a username fail (the exact exception may change in the future: gh-76912: Raise OSError from any failure in getpass.getuser() python/cpython#29739) I could wrap
getuserin a try/except block if that would be preferable? - As to the invalid filename, I suppose it could be a possibility. On all operating systems I'm aware of, users generally have directories that include the full username.
- Alternatively, perhaps these cached files don't belong in
/tmp(as ingettempdir) but rather the OS-specific user-cache directory (xdg or otherwise, using appdirs'suser_cache_dirfor example )
There was a problem hiding this comment.
Whether anything should be done here depends on whether or not this might fail on some platforms and how often this happens. I would say keep it like this, and we take another look if someone reports this breaking caching for them. But we should keep this in the back of our minds.
|
|
||
| if FS.exists(cache_fn): |
There was a problem hiding this comment.
Removing the call to FS.exists might break the test suite. The MockFS.open function does not result in an exception being generated if the file is missing.
There was a problem hiding this comment.
A call to file.exists() and then subsequently open() is redundant and susceptible to a race condition.
It looks like the test suite would need reworking to appropriately handle this change.
Would you like prefer that a test suite fix be added to this PR, or would you prefer the exists() check to be re-added?
Edit: the fix is actually relatively minor. I'll make that change and will backpedal if desired.
There was a problem hiding this comment.
No, what you implemented is pretty much the same I also came to conclude to be the best solution.
``MockFS.open`` signature now matches ``lark.utils.FS.open``.
|
Anything else I can do to help move this along? |
|
@klauer Sorry, I've been a little busy. I'll try to look into it this weekend. |
|
Thanks for the PR! |
Context
Closes #1178
Description
This is my attempt at fixing the issue I reported (#1178) regarding cache files generated by different users on the same system.
Consider this a proposal - I'd be happy to adjust it to your preference.
Three main changes were made:
$USER(viagetpass.getuser()) to the cache filename (77d8277)try/exceptblock to include cache load /open(cache_fn)(1c36098)try/exceptblock and a similarlogger.exceptionmessage (835e692)