Skip to content

Conversation

@lunny
Copy link
Member

@lunny lunny commented Oct 15, 2025

This PR fixes a panic issue in the WaitGroup that occurs when Gitea is shut down using Ctrl+C.
It ensures that all active connection pointers in the server are properly tracked and forcibly closed when the hammer shutdown is invoked.
The process remains graceful — the normal shutdown sequence runs before the hammer is triggered, and existing connections are given a timeout period to complete gracefully.

This PR also fixes no logger writer problem. Now the log close will only be invoked when the command exit.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 15, 2025
@lunny lunny added type/bug and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 15, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Oct 15, 2025
@lunny lunny added backport/v1.25 and removed modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Oct 15, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 15, 2025

  1. Fixes #35648 seems to be a wrong reference, 35648 is for OAuth2
  2. I don't think it's right to keep doing "// Check for shutdown signal during startup", the "shutdown" can occur exactly after your check, then the check doesn't really help anything.
  3. For the connections, if you decided to use "connections map", what does wl.server.wg.Add(1) do? Does it still make sense?
    • wg.Wait is used for waiting for the open connections, so it is still useful (BUT not right)
    • Maybe the Add-After-Wait data-race still exists (much more rare than before, better than before): doShutdown -> setState(stateShuttingDown) -> new conn comes -> listener.Close() -> wg.Wait() -> new conn wg.Add()

@lunny
Copy link
Member Author

lunny commented Oct 16, 2025

  1. Fixes #35648 seems to be a wrong reference, 35648 is for OAuth2

  2. I don't think it's right to keep doing "// Check for shutdown signal during startup", the "shutdown" can occur exactly after your check, then the check doesn't really help anything.

  3. For the connections, if you decided to use "connections map", what does wl.server.wg.Add(1) do? Does it still make sense?

    • wg.Wait is used for waiting for the open connections, so it is still useful
    • Maybe the Add-After-Wait data-race still exists (much more rare than before, better than before): doShutdown -> setState(stateShuttingDown) -> new conn comes -> listener.Close() -> wg.Wait() -> new conn wg.Add()

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 16, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Oct 16, 2025
@wxiaoguang wxiaoguang force-pushed the lunny/fix_shutdown_issue branch from 45d862c to b70d961 Compare October 16, 2025 01:37
@wxiaoguang wxiaoguang marked this pull request as draft October 16, 2025 01:37
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 16, 2025

The checking could check the situation ctrl+c before web listening. I can try to reduce them.

You can reproduce doesn't mean the fix is right.

Once listener closed, there should be no new connection coming.

I am showing the "DATA-RACE". "listener.Close" and "Accept" are in different threads.

Anyway, talk is cheap, I showed you my code.

@wxiaoguang wxiaoguang force-pushed the lunny/fix_shutdown_issue branch from b70d961 to 035aa77 Compare October 16, 2025 01:43
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 16, 2025

This PR also fixes no logger writer problem. Now the log close will only be invoked when the command exit.
Fixes [no logger writer]: Queue "repo-archive" stops running #35551

Your change is also wrong. I have told you the root problem and it is not easy to fix. #35551 (comment)

@wxiaoguang wxiaoguang marked this pull request as ready for review October 16, 2025 17:45
@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 Oct 17, 2025
@lunny lunny added this to the 1.26.0 milestone Oct 17, 2025
Comment on lines +220 to +221
srv.lock.Unlock()
srv.connEmptyCond.Broadcast()
Copy link
Contributor

@yp05327 yp05327 Oct 24, 2025

Choose a reason for hiding this comment

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

I’m not familiar with the sync functions, so this is just a question.
Should Broadcast be called before Unlock?
In the code above, Broadcast is always called before Unlock.
I asked AI about it, and it said it isn’t safe, so I left this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe AI didn't read the Golang doc: https://pkg.go.dev/sync#Cond.Broadcast

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @yp05327 means it’s inconsistent because, on line 209, the Broadcast function is called before the lock is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked AI about it, and it said it isn’t safe, so I left this comment.

I think @yp05327 means it’s inconsistent because ....

He means it isn’t safe, not inconsistent

Does "not safe" mean "inconsistent" in your mind?

@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 Oct 25, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ctrl+C result in panic [no logger writer]: Queue "repo-archive" stops running Ctrl +C shudown Gitea caused WaitGroup panic

5 participants