fix: allow for multiple browser sources #191
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
cmd/tracking.go (2)
86-90: Approve: Improved channel safety with nil checks.The addition of nil checks before sending to channels in
matchChansis a good practice. It prevents potential panics that could occur when attempting to send to a nil channel.As a minor optimization, consider using a separate slice for non-nil channels to avoid checking for nil on every iteration:
var activeChans []chan model.Match for _, mc := range ch.matchChans { if mc != nil { activeChans = append(activeChans, mc) } } for _, mc := range activeChans { mc <- match }This approach would be more efficient if
onNewMatchis called frequently or ifch.matchChanscontains many elements.
98-100: Approve: Consistent nil checks for channel safety.The addition of nil checks in this loop is consistent with the changes made in the
onNewMatchfunction. This improves the overall safety of the code by preventing potential panics when sending to nil channels.For consistency and potential optimization, consider applying the same approach suggested for the
onNewMatchfunction:var activeChans []chan model.Match for _, mc := range ch.matchChans { if mc != nil { activeChans = append(activeChans, mc) } } for _, mc := range activeChans { mc <- match }This would centralize the nil checking logic and potentially improve performance if this loop is executed frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cmd/tracking.go (1 hunks)
- pkg/server/server.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/server/server.go (1)
83-84: Potential goroutine leak: Missing client disconnect handlingThe
handleStreamfunction listens indefinitely onsseChan. If the client disconnects, the server may not detect it immediately, potentially leading to a goroutine leak.Consider implementing context cancellation or detecting client disconnection to properly exit the goroutine.
| for _, sse := range b.sseChans { | ||
| if sse != nil { | ||
| sse <- matchJson | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential data race on sseChans: Concurrent access without synchronization
The sseChans slice is accessed concurrently by multiple goroutines without proper synchronization, which can lead to data races and undefined behavior.
- In the
Startmethod (lines 43-47), the goroutine iterates overb.sseChansto send data to each channel. - In the
handleStreammethod (line 85), new channels are appended tob.sseChans.
To prevent data races, synchronize access to sseChans using a mutex.
Apply this diff to add synchronization:
+import "sync"
type BrowserSourceServer struct {
matchChan chan model.Match
sseChans []chan []byte
+ mu sync.Mutex
lastMatch []byte
}
func (b *BrowserSourceServer) handleStream(w http.ResponseWriter, _ *http.Request) {
// ...
sseChan := make(chan []byte, 1)
defer close(sseChan)
+ b.mu.Lock()
b.sseChans = append(b.sseChans, sseChan)
+ b.mu.Unlock()
// ...
}
func (b *BrowserSourceServer) Start(ctx context.Context, cfg *config.Config) {
// ...
go func() {
for match := range b.matchChan {
// ...
+ b.mu.Lock()
for _, sse := range b.sseChans {
if sse != nil {
sse <- matchJson
}
}
+ b.mu.Unlock()
}
}()
// ...
}Also applies to: 85-85
| b.lastMatch = matchJson | ||
| sseChan := make(chan []byte, 1) | ||
| defer close(sseChan) | ||
| b.sseChans = append(b.sseChans, sseChan) |
There was a problem hiding this comment.
Connection cleanup: Remove closed channels from sseChans
When a client disconnects, the sseChan is closed but remains in b.sseChans, potentially leading to resource leaks and memory consumption over time. Consider removing the closed channel from b.sseChans to prevent this.
Apply this diff to remove closed channels:
func (b *BrowserSourceServer) handleStream(w http.ResponseWriter, _ *http.Request) {
// ...
sseChan := make(chan []byte, 1)
defer func() {
+ b.mu.Lock()
+ // Remove sseChan from sseChans slice
+ for i, ch := range b.sseChans {
+ if ch == sseChan {
+ b.sseChans = append(b.sseChans[:i], b.sseChans[i+1:]...)
+ break
+ }
+ }
+ b.mu.Unlock()
close(sseChan)
}()
// ...
}Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
StartandhandleStreammethods to improve the flow and reliability of match data handling.