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

Prevent loop if there is an error in GetNotificationCount #19799

Conversation

zeripath
Copy link
Contributor

If the context is cancelled .NotificationUnreadCount in a template can
cause an infinite loop with ctx.ServerError() being called, which
creates a template that then calls .NotificationUnreadCount calling
GetNotificationCount() with the cancelled context resulting in an
error that calls ctx.ServerError... and so on...

This PR simply stops calling ctx.ServerError in the error handler code
for .NotificationUnreadCount as we have already started rendering and
so it is too late to call ctx.ServerError. Additionally we skip
logging the error if it's a context cancelled error.

Fix #19793

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

If the context is cancelled `.NotificationUnreadCount` in a template can
cause an infinite loop with `ctx.ServerError()` being called, which
creates a template that then calls `.NotificationUnreadCount` calling
`GetNotificationCount()` with the cancelled context resulting in an
error that calls `ctx.ServerError`... and so on...

This PR simply stops calling `ctx.ServerError` in the error handler code
for `.NotificationUnreadCount` as we have already started rendering and
so it is too late to call `ctx.ServerError`. Additionally we skip
logging the error if it's a context cancelled error.

Fix go-gitea#19793

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels May 24, 2022
@zeripath zeripath added this to the 1.17.0 milestone May 24, 2022
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Wonder if there are more similar cases

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 24, 2022
@zeripath
Copy link
Contributor Author

Wonder if there are more similar cases

searching through the codebase for .Data\[.*func doesn't reveal any others, and the FuncMap doesn't call it either.

It genuinely looks like this is the only place - but obviously we need to be more careful in future.

@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 25, 2022
@lunny lunny merged commit 93ab907 into go-gitea:main May 25, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 25, 2022
* giteaofficial/main:
  Prevent loop if there is an error in GetNotificationCount (go-gitea#19799)
  Add 'make tidy' (go-gitea#19800)
  Update document to use FHS `/usr/local/bin/gitea` instead of `/app/...` for Docker (go-gitea#19794)
@lunny lunny added the backport/done All backports for this PR have been created label May 25, 2022
@zeripath zeripath deleted the fix-19793-prevent-loop-if-error-in-GetNotificationCount branch May 25, 2022 21:07
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…9799)

If the context is cancelled `.NotificationUnreadCount` in a template can
cause an infinite loop with `ctx.ServerError()` being called, which
creates a template that then calls `.NotificationUnreadCount` calling
`GetNotificationCount()` with the cancelled context resulting in an
error that calls `ctx.ServerError`... and so on...

This PR simply stops calling `ctx.ServerError` in the error handler code
for `.NotificationUnreadCount` as we have already started rendering and
so it is too late to call `ctx.ServerError`. Additionally we skip
logging the error if it's a context cancelled error.

Fix go-gitea#19793

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

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@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
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea 1.17-dev crashing Out of memory
6 participants