fix: goroutine leak#41
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a goroutine leak in the local SSE stream implementation when the context is cancelled before (or around) signaling connect/disconnect/error events, which previously could leave goroutines blocked on unbuffered channel sends.
Changes:
- Buffer internal signal channels (
connectCh,esConnectErrCh,esDisconnectCh) to reduce the risk of blocked sends. - Make connect/disconnect callback signaling context-aware via
select(removing the previous TOCTOU pattern and eliminating the connect goroutine spawn). - Guard the subscribe error path with a
selectonctx.Done()to avoid blocking when the main loop has already exited.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.OnConnect(func(s *sse.Client) { | ||
| select { | ||
| case <-ctx.Done(): // Cancelled. | ||
| return | ||
| default: | ||
| go func() { connectCh <- true }() | ||
| case connectCh <- true: // Non-blocking due to buffer; no goroutine spawn needed. | ||
| } |
There was a problem hiding this comment.
There are existing stream_test.go tests, but none appear to cover the specific regression being fixed: context cancellation happening before/around the connect/disconnect callbacks, followed by the callback attempting to signal on the channel. Adding a test that cancels the stream and then invokes onConnCb/onDisCb (asserting it returns promptly and no goroutine is left blocked) would help ensure the leak doesn’t regress.
There was a problem hiding this comment.
Yeah, this is pretty hard to test. Let me see if LLM can get sth.
## [1.10.1](v1.10.0...v1.10.1) (2026-03-06) ### Bug Fixes * goroutine leak ([#41](#41)) ([c8912ff](c8912ff))
|
🎉 This PR is included in version 1.10.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Fixes go routine leak if the context is done before channel send, causing go routine to block indefinitely.
Checklist