-
-
Notifications
You must be signed in to change notification settings - Fork 903
Set Redis TTL on tab storage keys #5594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set Redis TTL on tab storage keys #5594
Conversation
evnchn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surface-level-speaking, let's use set for expiry if we really want to handle expiry in Redis.
And, generally we do not prefer to pass a parameter around for single-purpose. RedisPersistentDict can read core.app.storage.max_tab_storage_age (not Storage.max_tab_storage_age, let's align with the codebase) directly.
But I want to ask: Is that really a good idea?
Tracing back max_tab_storage_age, it is originally exclusively used in:
Lines 246 to 254 in dc49827
| async def prune_tab_storage(*, force: bool = False) -> None: | |
| """Prune tab storage that is older than the configured ``max_tab_storage_age``.""" | |
| tab_storages = core.app.storage._tabs # pylint: disable=protected-access | |
| for tab_id, tab in list(tab_storages.items()): | |
| if force or time.time() > tab.last_modified + core.app.storage.max_tab_storage_age: | |
| tab.clear() | |
| if isinstance(tab, PersistentDict): | |
| await tab.close() | |
| del tab_storages[tab_id] |
and notably: it is invoked every 10 seconds as seen in:
Line 146 in dc49827
| app.timer(10, prune_tab_storage) |
Then, I have the following questions:
- What happens to the tab storage, if Redis tossed the key because it is sharp-pass the deadline, but NiceGUI did not prune it due to the polling interval?
- Should, in the next polling interval, the tab storage is expired, does the pruning still occur normally?
- Should the tab storage is further mutated before the next NiceGUI poll occurs, then isn't the state torn between Redis and NiceGUI? Are there side-effects?
- Owing to the above concerns, is it better to handle the purging of Redis tab storage at
prune_tab_storageinstead?
Thank you for reading my longwinded comment!
|
Sorry, did I scare you away? 🥺 |
|
Apologies, I was off for the holidays. I've chosen the parameter approach because to me the alternative was too implicit. To address your feedback I added a flag based on the id prefix to avoid an explicit parameter, but I'm open for other ideas. To answer your question is this a good idea, to me this approach seems very reasonable. We have a Redis server that we have to reboot from time to time due to is running out of memory because of the orphaned tab storage keys. When using Redis storage, if a NiceGUI server crashes or restarts:
The drift between the expiration time counted in Python vs in Redis should be pretty close because they both update on writes. If you think this is still a concern we could add a small delta (max_tab_storage_age + delta) that is greater than polling interval when saving to Redis, this way if it's not pruned by that time, it is safe to assume it is orphaned. |
Checking the source code of the `redis` library, since `ex: Optional[ExpiryT] = None`, we can pass `None` if we do not want to set `ex`
Rationale: 1 cycle is definitely not enough due to potentially last-modified being 70 seconds: - NiceGUI's not pruning it - Redis, due to network latency, gets the request at 70.1 seconds, expired 20 seconds is a really conservative setting considering that attempts to connect to a Redis server with 10 seconds of network latency will likely fail horribly, but better safe than sorry!
|
Thanks. It seems that using I simplified the code a bit, and added a delta. Rationale for the delta value chosen is in the commit message of f643226
@falkoschindler Should be ready for review! |
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request, @quantumdark!
I just reviewed it thoroughly and came up with one small improvement: redist_persistent_dict.py and storage.py were coupled in a very fragile way based on the suffix "tab-". Instead I made ttl a parameter to be is set by storage.py. Furthermore I declared modul-level constants for various strings to reduce the risk for regressions.
Ready to merge! 👍🏻
evnchn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like my guessing of preference was wrong in:
And, generally we do not prefer to pass a parameter around for single-purpose.
RedisPersistentDictcan readcore.app.storage.max_tab_storage_age(notStorage.max_tab_storage_age, let's align with the codebase) directly.
I like it either way, so we can proceed.
Bonus item (non-blocker): I wonder is it a good idea to add a TTL to FilePersistentDict, so that old .json files on disk also gets purged? Right now I do not see a mechanism for purging old user JSON files...
Motivation
When using Redis as a storage backend (NICEGUI_REDIS_URL), tab storage keys accumulate forever in Redis. The existing prune_tab_storage() function only clears keys from the in-memory _tabs dictionary. If the application restarts, _tabs is empty and the app has no knowledge of orphaned Redis keys, causing them to persist until Redis runs out of memory (OOM).
Related to the max_tab_storage_age configuration which currently only works for in-memory cleanup but has no effect on Redis key expiration.
Implementation
Added TTL (Time To Live) support to RedisPersistentDict for tab storage:
Key behavior:
Progress