fix: prevent data race in tdsBuffer from concurrent processSingleResponse goroutines#357
Open
dlevy-msft-sql wants to merge 6 commits into
Open
fix: prevent data race in tdsBuffer from concurrent processSingleResponse goroutines#357dlevy-msft-sql wants to merge 6 commits into
dlevy-msft-sql wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #357 +/- ##
===========================================
+ Coverage 80.62% 96.60% +15.98%
===========================================
Files 35 92 +57
Lines 6910 74351 +67441
===========================================
+ Hits 5571 71829 +66258
- Misses 1068 2187 +1119
- Partials 271 335 +64
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
b8420f2 to
e1b94e3
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a go test -race data race in the TDS response-reading path by ensuring only one processSingleResponse goroutine can read from a session’s shared tdsBuffer at a time (fixes #171).
Changes:
- Add
readDone chan struct{}totdsSessionto track completion of the active response-reader goroutine. - Introduce
(*tdsSession).startResponseReader(...)to wait for the previous reader to exit before starting the next one, and use it from bothstartReading()and the cancellation retry path innextToken(). - Add a unit test intended to validate the new serialization behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tds.go |
Adds tdsSession.readDone used as a completion gate for response readers. |
token.go |
Centralizes spawning of processSingleResponse via startResponseReader, serializing readers to prevent concurrent buffer reads. |
token_test.go |
Adds a test for response-reader serialization behavior. |
Use a blockingTransport that holds the first reader goroutine in-flight, then assert that the second startResponseReader call blocks until the first completes. This catches the actual race condition instead of testing the already-finished path.
Replace time.Sleep with readEntered channel for deterministic assertion that the first reader is blocked. Fix comment to accurately describe EOF error path (not panic recovery).
1f77eed to
3fdaa3f
Compare
…Serializes Addresses reviewer feedback: the assertion that secondStarted is still 0 could be a false positive if the goroutine wasn't scheduled yet. Now we wait for a goroutine-started channel plus a 100ms grace period before checking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prevents the data race in
tdsBuffercaused by concurrentprocessSingleResponsegoroutines reading from the same session buffer.Problem
When an
INSERT...OUTPUTor similar query produces results,startReading()spawns aprocessSingleResponsegoroutine that reads fromsess.buf. If the caller doesn't fully drain the rows before executing the next statement,startReading()spawns a second goroutine that reads from the samesess.bufconcurrently. Both goroutines write totdsBuffer.rpos,tdsBuffer.rsize, etc. without synchronization, causing a data race detected bygo test -race.Fix
Add a
readDone chan struct{}field totdsSession. EachprocessSingleResponsegoroutine closes itsreadDonechannel on exit (viadefer close(readDone)). The nextstartReading()call waits on the previousreadDonechannel before spawning a new goroutine.The same gate is applied to the cancellation retry path in
nextToken()where a secondprocessSingleResponsegoroutine is spawned to read the cancellation confirmation.Why this is safe
readDoneisnilon the first call, so no blocking occursstartReadingis only called from the connection's serial execution path, so no mutex is neededdefer close(readDone)to guarantee the channel is always closed, even on panic recoveryRisk
Low. The wait only adds latency in the exact scenario that causes the data race today.
Fixes #171