Fix send on closed channel panic in SSE stream handlers#6456
Conversation
Instead of explicitly closing channels which leads to concurrency issues with channels being used after close, allow channels to be GC'ed. Remove the recover block that was previously catching&masking the panics in the LogStreamSSE. EventStreamSSE was missing the same fixes: woodpecker-ci#6454
for more information, see https://pre-commit.ci
utafrali
left a comment
There was a problem hiding this comment.
The core fix is correct and is a genuine improvement: removing close(eventChan) / close(logChan) from defers eliminates the send-on-closed-channel panics, the updated selects properly unblock on context cancellation (better than the old default-fallthrough pattern), and adding close(batches) fixes a pre-existing goroutine leak in LogStreamSSE. The main concerns are test fragility (timing-based sync) and a potential last-entries-lost race when a log stream ends naturally before the inner goroutine drains its buffer.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
now I dont know why there are tow pulls working on the same issue by the same people either authored or reviewed!?!? -> #6455 |
How you wish to proceed @6543 I suspected the another PR was agent generated as the unit tests still weren't testing against the right fix, and the further commits introduced more regression. Happy to close this one, as the other PR has a good start. |
|
well i prevere human interaction, if llms are used is fine as long as it's just a tool, the contributor has to own its mistakes and also understand the code ... I will look into both pulls and then decide what to take from what ... give me time to do so :) |
Co-Authored-By: utafrali <tafraliugur@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6456 +/- ##
==========================================
+ Coverage 40.87% 41.32% +0.44%
==========================================
Files 431 431
Lines 28821 28815 -6
==========================================
+ Hits 11781 11907 +126
+ Misses 15987 15840 -147
- Partials 1053 1068 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Instead of explicitly closing channels which leads to concurrency issues with channels being used after close, allow channels to be GC'ed.
Remove the recover block that was previously catching&masking the panics in the LogStreamSSE. EventStreamSSE was missing the same
fixes: #6454