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

IUserTimeZoneService no longer caches user requests #16932

Open
deanmarcussen opened this issue Oct 28, 2024 · 5 comments
Open

IUserTimeZoneService no longer caches user requests #16932

deanmarcussen opened this issue Oct 28, 2024 · 5 comments
Milestone

Comments

@deanmarcussen
Copy link
Member

this pr #16614 has made some changes to how the users time zones are cached which makes the cache useless.

this code in UserTimeZoneSelector now retrieves the current user on every request, before trying to get a timezone for the user.

(or in fact if a timezone calcation is done more than once on a page, which is a common usage, then it queries for the user multiple times)

        var currentUser = await _userManager.GetUserAsync(_httpContextAccessor.HttpContext.User);

the previous arrangement where UserTimeZoneService held the cache for the database query to lookup the user (to get their timezone) was a useful cache. Now it's irrelevant, because the componet calling into that cache is doing all the work.

@Skrypt
Copy link
Contributor

Skrypt commented Oct 29, 2024

Remembers me something I had fixed once.

@Piedone Piedone added the perf label Oct 29, 2024
@sebastienros
Copy link
Member

We need to remove the cache from the IUserTimeZoneService implementation and move it to the UserTimezoneSelector instead, which will prevent it from loading the user. The key will be the username which is fetched from the HttpContext.

@sebastienros sebastienros added this to the 2.x milestone Oct 31, 2024
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@sebastienros
Copy link
Member

@deanmarcussen I suggest you watch today's meeting when it's available, we review the latest PR. My suggestion is to remove this service and put whatever it does (caching) in UserTimeZoneSelector. I explain my reasons during the meeting. The other option we have is to keep what @MikeAlhayek has done in the new PR, which fixes the caching issue (like my suggestion it's a breaking change).

@MikeAlhayek
Copy link
Member

@sebastienros additionally, removing the IUserTimeZoneService will be a breaking change and can't be done in 2.x. unless we want to leave it there as useless code for 2.x and remove it in 3.0.

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

5 participants