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

Use common sessioner for API and Web #17027

Merged
merged 3 commits into from
Sep 12, 2021

Conversation

zeripath
Copy link
Contributor

Instead of creating separate sessioner and doubly initialising the provider just
use the same sessioner for the API and Web routes.

Signed-off-by: Andrew Thornton art27@cantab.net

Instead of creating separate sessioner and doubly initialising the provider just
use the same sessioner for the API and Web routes.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the type/enhancement An improvement of existing functionality label Sep 12, 2021
@zeripath zeripath added this to the 1.16.0 milestone Sep 12, 2021
@zeripath
Copy link
Contributor Author

If we merge in https://gitea.com/go-chi/session/pulls/1 then this will fix a data-race detected in https://drone.gitea.io/go-gitea/gitea/43900/2/14

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 12, 2021
routers/init.go Show resolved Hide resolved
@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 Sep 12, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2021

Codecov Report

Merging #17027 (9905904) into main (aac7f68) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17027      +/-   ##
==========================================
- Coverage   45.23%   45.22%   -0.01%     
==========================================
  Files         766      766              
  Lines       86641    86632       -9     
==========================================
- Hits        39191    39179      -12     
+ Misses      41108    41107       -1     
- Partials     6342     6346       +4     
Impacted Files Coverage Δ
routers/api/v1/api.go 78.19% <100.00%> (-0.29%) ⬇️
routers/init.go 62.96% <100.00%> (+5.82%) ⬆️
routers/web/web.go 90.40% <100.00%> (-0.13%) ⬇️
modules/git/repo_base_nogogit.go 82.85% <0.00%> (-2.86%) ⬇️
modules/git/utils.go 68.05% <0.00%> (-2.78%) ⬇️
models/issue_comment.go 51.17% <0.00%> (-0.59%) ⬇️
modules/queue/workerpool.go 49.23% <0.00%> (+1.14%) ⬆️
modules/queue/queue_channel.go 96.66% <0.00%> (+1.66%) ⬆️

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 aac7f68...9905904. Read the comment docs.

@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 Sep 12, 2021
@6543 6543 merged commit a334a95 into go-gitea:main Sep 12, 2021
@zeripath zeripath deleted the fix-race-in-virtual-session-provider branch September 12, 2021 18:22
@lunny
Copy link
Member

lunny commented Sep 13, 2021

APIs should be split as two parts, one is only for web, another is real API. The real API should remove Sessioner.

zeripath added a commit to zeripath/gitea that referenced this pull request Sep 13, 2021
Update to latest go-chi/session where the NewManager causes a new
Provider instantiation instead of reconfiguring an old one.
(https://gitea.com/go-chi/session/pulls/1)

The NewManager call is now concurrency safe and would allow live
reconfiguration in future but for now this PR simply fixes an
intermittent "data-race" detected in our tests.  (See
https://drone.gitea.io/go-gitea/gitea/43900/2/14)

Related go-gitea#17027
Related go-gitea#1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this pull request Sep 13, 2021
Update to latest go-chi/session where the NewManager causes a new
Provider instantiation instead of reconfiguring an old one.
(https://gitea.com/go-chi/session/pulls/1)

The NewManager call is now concurrency safe and would allow live
reconfiguration in future but for now this PR simply fixes an
intermittent "data-race" detected in our tests.  (See
https://drone.gitea.io/go-gitea/gitea/43900/2/14)

Related #17027
Related #1441

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants