Skip to content
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

Clear user timezone cache when user is updated #15536

Open
deanmarcussen opened this issue Mar 18, 2024 · 5 comments
Open

Clear user timezone cache when user is updated #15536

deanmarcussen opened this issue Mar 18, 2024 · 5 comments
Milestone

Comments

@deanmarcussen
Copy link
Member

Related to the improvements to the user timezone cache in the UserTimeZoneService

private static readonly DistributedCacheEntryOptions _slidingExpiration = new() { SlidingExpiration = TimeSpan.FromHours(1) };

Because it's gone to sliding, if the user changes their timezone (we allow the user to manage it themselves as part of their user profile), I would suggest a user event handler, which would clear the cache when the user is updated, would be useful.

The current scenario, because it's a sliding expiration, means the user will never see a timezone change when they make their settings, until they stop for the day, and have no activity for +1 hour.

An absolute expiration might be a sensible backup option as well, in case the handler doesn't fire.

I will probably write one when we do our next upgrade to OC, so would commit it back, but that upgrade maybe a few more months away for us :)

@KeithRaven
Copy link
Contributor

@deanmarcussen can I pick this one up? :)

@sebastienros
Copy link
Member

@KeithRaven you got a thumbs up from @deanmarcussen

@KeithRaven
Copy link
Contributor

@deanmarcussen I've assumed that your updating user profiles through custom code as the UserTimeZoneDisplayDriver was already clearing the cache on update, so when using the built in UI it seemed fine.

I've added a handler to clear the cache on both update and delete of a user, as deleting and recreating a user (again not through the main UI) would also pick up the stale cached value. I've cleared the cache in the same way the driver did but removed that line from the driver as it now seemed superfluous.

I've not added an absolute expiration yet as I couldn't see under what circumstances the handler might not fire, perhaps you had something in mind?

@deanmarcussen
Copy link
Member Author

@KeithRaven yes, custom code on our front end does the updates, so outside the driver scopes.

An abolute expiration was yes, just in case the handler doesn't fire. I have been finding that with redis and a couple of servers things can get stuck there a bit easier, so I'm tending to always expire eventually, so things self heal. Not a major issue, just annoying when turning the server on and off again doesn't fix it!

@KeithRaven
Copy link
Contributor

@deanmarcussen So not so much the handler itself not firing but some distributed cache implementations maybe not 100% reliable when clearing?

I sat on it a few days as I was over thinking it wondering if it was the right place for a mitigation for particular cache providers, but I think making it configurable is probably overkill for such a small thing and so probably just a sensible default. Does 4hrs seem reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants