Make the background IP Rule-refreshing goroutine exit on server shutdown#11611
Make the background IP Rule-refreshing goroutine exit on server shutdown#11611iain-macdonald merged 8 commits intomasterfrom
Conversation
6ba1f75 to
c46377b
Compare
There was a problem hiding this comment.
Pull request overview
This PR ensures the background IP rules cache-refreshing goroutine terminates during server shutdown, avoiding lingering goroutines in long-running enterprise servers.
Changes:
- Add a shutdown handshake (
stop/done) for the IP rules refresher and register it with the server HealthChecker. - Add a goroutine-leak regression test using
go.uber.org/goleak. - Wire new dependencies through Go modules and Bazel.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds go.uber.org/goleak dependency for goroutine leak testing. |
| enterprise/server/ip_rules_enforcer/ip_rules_enforcer_test.go | Adds a goleak-based test asserting the refresher stops on shutdown. |
| enterprise/server/ip_rules_enforcer/ip_rules_enforcer.go | Implements stoppable refresher goroutine and HealthChecker shutdown integration. |
| enterprise/server/ip_rules_enforcer/BUILD | Adds Bazel deps for proto and goleak. |
| deps/go_deps.MODULE.bazel | Exposes org_uber_go_goleak repo for Bazel builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| } | ||
| stop := make(chan struct{}) | ||
| done := make(chan struct{}) | ||
| var shutdownOnce sync.Once |
There was a problem hiding this comment.
nit: you can make this a bit neater by doing:
closeStop := sync.OnceFunc(func() {close(stop)})
Then you can just call pass that to shutdownRefresher, instead of passing both stop and shutdownOnce. Or you can even just call closeStop() before calling shutdownRefresher, since it's pretty weird to pass in a function just to call once it right away. Then maybe shutdownRefresher should be called waitForShutdown.
There was a problem hiding this comment.
Good idea, fixed
No description provided.