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

Fix cocurrent upload package #21836

Closed
wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 16, 2022

When inserting failed, then check if the record does exist, if yes, then just return the record duplicated.

@lunny lunny added the type/bug label Nov 16, 2022
Comment on lines +139 to 146
has, err := e.Exist(key)
if err != nil {
return nil, err
}
if has {
return key, ErrDuplicatePackage
}
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's wrong. You might get has=false, err=nil then return nil, nil

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 16, 2022
@@ -128,14 +128,21 @@ func TryInsertPackage(ctx context.Context, p *Package) (*Package, error) {
LowerName: p.LowerName,
}

has, err := e.Get(key)
has, err := e.Exist(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong because Exist does not populate the object with the content from the database.

@KN4CK3R
Copy link
Member

KN4CK3R commented Nov 16, 2022

And this doesn't fix the current problem because the error does not occur in these methods but when the transaction is committed.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 17, 2022

And this doesn't fix the current problem because the error does not occur in these methods but when the transaction is committed.

Although I haven't looked into the problem, I think using transaction correctly will resolve the concurrency problem.

Database transactions will acquire write lock for all DML (insert/update/...) statements, then other DML in other transactions will block and wait.

@wxiaoguang
Copy link
Contributor

@lunny lunny closed this Nov 19, 2022
@lunny lunny deleted the lunny/fix_cocurrent_upload_package branch November 19, 2022 02:23
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants