Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix system config cache expiration timing #28072

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

wxiaoguang
Copy link
Contributor

To avoid unnecessary database access, the cacheTime should always be set if the revision has been checked.

Fix #28057

@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Nov 15, 2023
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Nov 15, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 15, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 15, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 15, 2023
@lng2020
Copy link
Member

lng2020 commented Nov 16, 2023

Since the first read lock is only used for d.revision and d.cacheTime, why not

 func (d *dbConfigCachedGetter) GetRevision(ctx context.Context) int {
-       d.mu.RLock()
-       defer d.mu.RUnlock()
+       d.mu.Lock()
+       defer d.mu.Unlock()
        if time.Since(d.cacheTime) < time.Second {
                return d.revision
        }
-       d.mu.RUnlock()
-       d.mu.Lock()
        if GetRevision(ctx) != d.revision {
                rev, set, err := GetAllSettings(ctx)
                if err != nil {
@@ -131,8 +129,6 @@ func (d *dbConfigCachedGetter) GetRevision(ctx context.Context) int {
                }
        }
        d.cacheTime = time.Now()
-       d.mu.Unlock()
-       d.mu.RLock()
        return d.revision
 }

to simplify code. I guess it's not very harmful to the performance?

@wxiaoguang
Copy link
Contributor Author

I think the "read lock" is still good to have.

To simplify it, it could be like the last commit.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 16, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 16, 2023
@lunny lunny enabled auto-merge (squash) November 16, 2023 12:30
@lunny lunny merged commit fce1d5d into go-gitea:main Nov 16, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 16, 2023
To avoid unnecessary database access, the `cacheTime` should always be
set if the revision has been checked.

Fix go-gitea#28057
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Nov 16, 2023
@wxiaoguang wxiaoguang deleted the fix-setting-cache branch November 16, 2023 13:33
wxiaoguang added a commit that referenced this pull request Nov 16, 2023
Backport #28072

To avoid unnecessary database access, the `cacheTime` should always be
set if the revision has been checked.

Fix #28057

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 17, 2023
* giteaofficial/main:
  Fix incorrect pgsql conn builder behavior (go-gitea#28085)
  Fix system config cache expiration timing (go-gitea#28072)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
To avoid unnecessary database access, the `cacheTime` should always be
set if the revision has been checked.

Fix go-gitea#28057
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 14, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caching issue is back
4 participants