fix(Srv/stdio): risk of goroutine leaks and concurrent reads in readNextLine()
#257
Conversation
WalkthroughAn explicit return statement was added after sending a read line to a channel within a goroutine in the Changes
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
readNextLine()readNextLine()
readNextLine()readNextLine()
|
Hi @cryo-zd, thank you for the detailed explanation of the concurrency issue — I really appreciate the insights. 🙏 I’m trying to fully understand the scenario you described and would like to clarify one point: From the implementation of readNextLine, the spawned goroutine appears to execute ReadString('\n') once, then attempts to write the result to readChan or errChan, and then it returns (or at least, there's no loop). So it seems like the goroutine should terminate after that. However, the comment mentions: “If the Go scheduler delays the main thread’s consumption of the value, the same goroutine may continue running and call ReadString('\n') again.” Could I ask for clarification here? What exactly does "the same goroutine may continue running" mean in this context? Since the goroutine isn’t written with a loop, how would it end up calling ReadString more than once? Thanks again for your time and help — I just want to make sure I’m not missing something subtle here. |
oh, yes, I made a mistake here, |
Problem Summary
The original implementation of
readNextLinespawns a goroutine to perform aReadString('\n')call and deliver the result viareadChanorerrChan. However, this goroutine did not return immediately after writing to a channel, which introduces two subtle concurrency issues caused by incorrect assumptions about goroutine scheduling and lifecycle.[Problem]Data Loss or Goroutine Leak
If the outer
readNextLinefunction exits due to context cancellation (e.g.,ctx.Done()), but the inner goroutine is still blocked onReadStringor attempting to send to a now-unreadreadChan, the following may happen:readChanis never read again,➡️ Result: The read data is lost, and the goroutine leaks, causing silent memory and input channel pressure over time.
[Problem] Concurrent Reads on
bufio.Reader:If the spawned goroutine successfully delivers a line to
readChanbut does not return immediately, it may remain alive and proceed to callReadString('\n')again. This can happen if the Go scheduler delays the main thread’s consumption of the value, allowing the same goroutine to continue running.➡️ Result: Multiple goroutines may perform concurrent reads on the shared
bufio.Reader, which is not safe for concurrent use. This can cause data corruption, misordered reads, or panics.Root Cause: Invalid Timing Assumption
The original logic assumes:
This is a scheduling-dependent assumption, and does not hold reliably in Go's concurrent model. The race occurs in the tiny gap between delivering the line and reacting to
done.Fix: Exit Goroutine Immediately After Attempting to Deliver
We fix this by ensuring the goroutine returns immediately after the
selectthat attempts to deliver the line,:This ensures:
Summary by CodeRabbit