Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
e419435 to
7d5aff6
Compare
fd96e57 to
181d80b
Compare
| s.serverMtx.RLock() | ||
| defer s.serverMtx.RUnlock() |
There was a problem hiding this comment.
The mutex+nil-check in the old shutdown goroutines was needed because those goroutines ran concurrently with server setup; the server field might not have been assigned yet when ctx.Done() fired. In the new code, serveWithShutdown is only called after the server is already assigned and receives it as a direct argument, so the race doesn't exist.
| } | ||
|
|
||
| func (s *ConnectService) startServer(svcConf service.Configuration) error { | ||
| func serveWithShutdown(ctx context.Context, server *http.Server, serveFn func() error) error { |
There was a problem hiding this comment.
I extracted this because we use it in a couple places to handle server shutdowns.
|
/gemini review |
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
181d80b to
a37ab39
Compare
|
There was a problem hiding this comment.
Code Review
This pull request refactors the server lifecycle management in ConnectService by introducing a serveWithShutdown helper function, which simplifies the Serve method and centralizes graceful shutdown logic. Feedback was provided regarding a potential race condition in serveWithShutdown where a server error could be masked if the context is cancelled at the same time, suggesting a check for errors after the shutdown process completes.



Fixes an issue where a failure of the metrics server to start can cause a zombie process.
This is hard and relatively low value to test with unit tests, but I was able to easily reproduce it with this simple script:
After the simple context handling, the script confirms the bug is fixed.
Fixes: #1807