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

Change createRepository to call PrepareWebhooks without xorm session #7447

Closed
wants to merge 1 commit into from
Closed

Conversation

davidsvantesson
Copy link
Contributor

@davidsvantesson davidsvantesson commented Jul 12, 2019

Changes the call in createRepository from prepareWebhooks to PrepareWebhooks, without the xorm session as argument. This is the same call as used in for example DeleteRepository.
I have tested this against issue #7404 and it should be solved with this fix.

… to ensure transaction is directly.

Signed-off-by: David Svantesson <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #7447 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7447      +/-   ##
==========================================
- Coverage    41.2%   41.19%   -0.01%     
==========================================
  Files         469      469              
  Lines       63557    63557              
==========================================
- Hits        26190    26185       -5     
- Misses      33946    33950       +4     
- Partials     3421     3422       +1
Impacted Files Coverage Δ
models/repo.go 48.6% <0%> (ø) ⬆️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/issue.go 48.74% <0%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0018d56...2051e8e. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 12, 2019
@lunny
Copy link
Member

lunny commented Jul 13, 2019

This is for avoiding multiple database connections since sqlite may lock.

@davidsvantesson
Copy link
Contributor Author

The problem here is that the commit to sql database (of the hook_task) has to be done before the webhook is added to the queue. Otherwise the webhook might not be delivered (DeliverHooks in webhook.go read nothing from sql).

If it is a problem to have multiple sql sessions at the same time, a larger change than what I have proposed would be needed. Maybe the PrepareWebhooks can move to the end of createRepository (as for delete repo) after closing sql session. However createRepository is also used by ForkRepository so it is not that easy.

@lunny
Copy link
Member

lunny commented Jul 15, 2019

Yes, this is the bug, but your change will introduce another problem. I think we have to move go HookQueue.Add(repo.ID) outside the database transaction.

@davidsvantesson
Copy link
Contributor Author

Database session end in ForkRepository and CreateRepository. Drawback to move HookQueue.Add there is it will duplicate the call and maybe will make code somewhat less intuitive to follow. Not sure if there is some smarter way to refactor the code, or maybe that is acceptable solution?

@davidsvantesson
Copy link
Contributor Author

I close this PR as it wasn't a satisfactory solution for the issue.

@zeripath
Copy link
Contributor

Thanks for your time and contribution. (Also thanks for closing this PR once it was no longer necessary)

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants