@cachedList can insert None instead of absence of a key #17726
Comments
|
It could probably use a sentinel instead of None to know if it uncached? |
|
Maybe the mypy plugin is missing this coercion? |
|
That should probably handle |
That's what I had to do for Related discussion: #17450 (comment) |
…ooms (#17727) Fixes element-hq/element-x-ios#3300 Some rooms are missing from `sliding_sync_joined_rooms`. When this happens, the first call will succeed, but any subsequent calls for this room ID will cause the cache to return `None` for the room ID, rather than not having the key at all. This then causes the `<=` check to throw. Root cause: #17726 ### Pull Request Checklist <!-- Please read https://element-hq.github.io/synapse/latest/development/contributing_guide.html before submitting your pull request --> * [x] Pull request is based on the develop branch * [ ] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [ ] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters))
|
I think the historical context for this is that it was very much expected that a value was returned for every key, i.e. the bulk fetch function should return the exact same thing as just calling the non-bulk function in a loop. The common case for store functions that couldn't find a row is to return None, and so that is likely why we treated missing values as None. However, there are now quite a few functions that don't actually have a non-bulk equivalent, and so it could make sense for those functions not to return everything. Agreed this is a footgun, and we probably should make |
The docs for this state:
This is not semantically the same as absence of a key, and creates very subtle failure modes. I just spent an hour or two trying to figure out how a function could possibly set None and well, it can't. It turns out the cache is doing it because some rooms don't have entries in the DB. The function in question is
synapse/synapse/storage/databases/main/stream.py
Line 1495 in 51dd4df
Unfortunately, the caller of this function never did a None check causing runtime failures: see element-hq/element-x-ios#3300 (comment)
It is absolutely not clear that adding a cache decorator would change the semantics like this. I suggest we double check every caller of these functions to make sure they
Nonecheck before usage.The text was updated successfully, but these errors were encountered: