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 route rather than use thus reducing the number of stack frames #15301

Merged
merged 11 commits into from
May 4, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Apr 6, 2021

Since the move to Chi the number of stack frames has proliferated somewhat catastrophically and we're up to 96 frames with multiple tests of the url outside of a trie which is inefficient.

This PR reduces the number of stack frames by 6 through careful use of Route, moves Captcha into its own router so that it only fires on Captcha routes, similarly for avatars and repo-avatars.

The robots.txt, / and apple-touch-icon.png are moved out of requiring Contexter.

It moves access logger higher in the stack frame because there is no reason why it can't be higher.

Extract from #15186
Contains #15292

@zeripath zeripath added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 6, 2021
@zeripath zeripath added this to the 1.15.0 milestone Apr 6, 2021
// this png is very likely to always be below the limit for gzip so it doesn't need to pass through gzip
routes.Get("/apple-touch-icon.png", func(w http.ResponseWriter, req *http.Request) {
http.Redirect(w, req, path.Join(setting.StaticURLPrefix, "img/apple-touch-icon.png"), 301)
})
Copy link
Member

Choose a reason for hiding this comment

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

Can probably nuke that route and make it use a <link> tag to directly link to the image. I recall this route was only there for some edge case with Firefox, but it should be favoring the SVG icon over this one nowadays.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 6, 2021
@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 4, 2021
@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 4, 2021
@zeripath
Copy link
Contributor Author

zeripath commented May 4, 2021

Make lgtm work

@zeripath zeripath merged commit 47fd156 into go-gitea:main May 4, 2021
@zeripath zeripath deleted the use-route-rather-than-use branch May 4, 2021 21:48
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

after upgrading to this the user-specified avatars won't load.. I'm not sure if that's intentional, it's potentially kind/breaking.

EDIT: org avatars won't load at all (no matter if user-specified or gravatar)
EDIT2: org avatars that should be loaded from gravatar are grabbed from local instance, (/avatars/jfkdjfsdkfjksfkjsdf), which, of course, fails (unless there's supposed to be a redirection to gravatar in the background that I'm not aware of)

@zeripath
Copy link
Contributor Author

zeripath commented May 4, 2021

Not intentional at all - I thought I'd tested all of this.

It might be that it's reverted your recent fix as this was written before it.

Do you remember your pr that fixed things?

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

Not intentional at all - I thought I'd tested all of this.

No worries.

It might be that it's reverted your recent fix as this was written before it.

Do you remember your pr that fixed things?

There was this thing with assets, should I check that?
I'll go revert that my fix (moving things to /assets/{img,css,*}/...) and report back.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

looks like custom css still works fine and so do custom /assets-based icons.
this is different, though, this does not pertain customizations, rather /avatars stuff

so the answer is I don't recal what PR could be the partial cause other than the assets one @zeripath
the issue is as described above, org avatars that should be fetched from gravatar are looked for locally, user avatars that are user-supplied are 404s.

I do think this is really worth doing so fixing the problem rather than reverting this pr should be the aim.

I am all for it.

prometheus metrics look fine, though :D

@zeripath
Copy link
Contributor Author

zeripath commented May 4, 2021

Unfortunately there was quite a few prs between this being written and it getting approved - I bet something to do with the assets move is the cause.

I should have rechecked before merging.

I do think this is really worth doing so fixing the problem rather than reverting this pr should be the aim.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

I have done a quick write-up of the issue so that I don't pollute this PR anymore

zeripath added a commit to zeripath/gitea that referenced this pull request May 5, 2021
There was a missing * from the avatars routes in go-gitea#15301.

Fix go-gitea#15727

Signed-off-by: Andrew Thornton <[email protected]>
zeripath added a commit that referenced this pull request May 5, 2021
There was a missing * from the avatars routes in #15301.

Fix #15727

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath mentioned this pull request May 9, 2021
12 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants