Fix listener resource leaks in serve commands#1945
Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Assessment: 🟢 APPROVE
This PR successfully fixes the listener resource leaks by refactoring listenAndCloseOnCancel into newListener with a proper cleanup function pattern. The implementation correctly uses context.AfterFunc and its returned stop function to ensure proper synchronization and prevent resource leaks.
Key improvements:
- ✅ Replaces raw goroutine with
context.AfterFunc, eliminating goroutine leak potential - ✅ Cleanup function properly calls
stop()first to prevent callback execution - ✅ All callers consistently use
defer cleanup()pattern - ✅ The
stop()function provides the necessary synchronization to prevent race conditions
Analysis:
The pattern of calling stop() followed by ln.Close() is correct and thread-safe. The stop() function from context.AfterFunc ensures that either the callback hasn't run (and won't run), or it already ran. This eliminates any race condition concerns around double-closing the listener.
No bugs found in the changed code. The refactoring is clean and follows Go best practices for resource cleanup with context cancellation.
Note: This review analyzed only the added/modified code in this PR.
Rename listenAndCloseOnCancel to newListener and have it return a cleanup function alongside the listener. Callers now defer the cleanup function, which ensures the listener is always closed on return, regardless of whether the context was cancelled or the downstream Serve/Run call returned an error. Previously, the helper only closed the listener on context cancellation (via a goroutine), leaving a resource leak if the server returned an error before the context was cancelled. The callers had to remember to add their own defer ln.Close() — a responsibility that was easy to miss. Now the cleanup is centralized in newListener. This also fixes a2a.go which had the same missing defer ln.Close(). Fixes docker#1934 Fixes docker#1935 Assisted-By: cagent
dgageot
force-pushed
the
board/docker-agent-issues-cb91df7b
branch
from
cc74347 to
cb7b342
Compare
2 participants
Refactor
listenAndCloseOnCancelintonewListener, which returns a cleanup function alongside the listener. Callers nowdefer cleanup(), ensuring the listener is always closed on return — whether the context was cancelled or the downstream Serve/Run call returned an error.Changes:
context.AfterFunc, eliminating a potential goroutine leak when the server exits before context cancellation.stop()(to cancel the context callback if it hasn't fired) then closes the listener.mcp.go,api.go,a2a.go) now use the samedefer cleanup()pattern.Fixes #1934
Fixes #1935