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

Remove the parallelizing when loading repo for dashboard #24705

Merged
merged 5 commits into from
May 14, 2023

Conversation

wxiaoguang
Copy link
Contributor

Ref: #24638

IMO, parallelizing might run out server resources more quickly. Gitea shouldn't use a lot of go-routine in a web handler.

And add a comment about how many repositories there could be at most.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 14, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 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 May 14, 2023
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label May 14, 2023
repoIDsToLatestCommitSHAs := make(map[int64]string)
wg := sync.WaitGroup{}
wg.Add(len(repos))
// at most there are dozens of repos (limited by MaxResponseItems), so it's not a big problem at the moment
Copy link
Member

Choose a reason for hiding this comment

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

(which is limited by RepoPagingNum)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure?

The PageSize comes from PageSize: convert.ToCorrectPageSize(ctx.FormInt("limit")),, and ToCorrectPageSize says:

else if size > setting.API.MaxResponseItems {
		size = setting.API.MaxResponseItems
	}

Copy link
Member

Choose a reason for hiding this comment

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

I see, limit is RepoPagingNum but this condition overrides it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily it's checked, that's what I have double-checked.

Otherwise, attackers could to "limit=99999" to DoS the server.

Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

It's fine if you want it serialized, there is such a small number of calls it doesn't matter you are right, the previous code was unsafe

@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 May 14, 2023
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 14, 2023
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

Skip errors, don't include them

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels May 14, 2023
@yardenshoham yardenshoham removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 14, 2023
Co-authored-by: Yarden Shoham <git@yardenshoham.com>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels May 14, 2023
@yardenshoham
Copy link
Member

Sorry for that, I assumed map was safe

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 14, 2023
Co-authored-by: Yarden Shoham <git@yardenshoham.com>
@lunny lunny merged commit 3af2c8e into go-gitea:main May 14, 2023
@GiteaBot GiteaBot added this to the 1.20.0 milestone May 14, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 14, 2023
@wxiaoguang wxiaoguang deleted the fix-dash-repo-go branch May 14, 2023 13:48
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 15, 2023
* upstream/main: (30 commits)
  Don't filter action runs based on state (go-gitea#24711)
  Add Go package registry (go-gitea#24687)
  Fix flash of unstyled content in action view page (go-gitea#24712)
  Clean up various avatar dimensions (go-gitea#24701)
  Remove the parallelizing when loading repo for dashboard (go-gitea#24705)
  Optimize actions list by removing an unnecessary `git` call (go-gitea#24710)
  Update cron-translations.yml (go-gitea#24708)
  Fix run list broken when trigger user deleted (go-gitea#24706)
  Remove Fomantic comment module (go-gitea#24703)
  Update to Alpine 3.18 (go-gitea#24700)
  fix minio storage iterator path (go-gitea#24691)
  Add status indicator on main home screen for each repo (go-gitea#24638)
  Add test for api team orgnization (go-gitea#24699)
  Improve button-ghost, remove tertiary button (go-gitea#24692)
  Add icon support for safari (go-gitea#24697)
  Improve avatar uploading / resizing / compressing, remove Fomantic card module (go-gitea#24653)
  Fix docs documenting invalid `@every` for `OLDER_THAN` cron settings (go-gitea#24695)
  Fix `organization` field being `null` in `GET /api/v1/teams/{id}` (go-gitea#24694)
  Use standard HTTP library to serve files (go-gitea#24693)
  Add `eslint-plugin-eslint-comments` (go-gitea#24690)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 12, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants