fix: remove rows.Err() from Scan to prevent deadlock (#7403)#7720
Open
DukeDeSouth wants to merge 1 commit intogo-gorm:masterfrom
Open
fix: remove rows.Err() from Scan to prevent deadlock (#7403)#7720DukeDeSouth wants to merge 1 commit intogo-gorm:masterfrom
DukeDeSouth wants to merge 1 commit intogo-gorm:masterfrom
Conversation
Scan() called rows.Err() after scanning, but rows.Err() acquires an RLock on the internal closemu RWMutex. When context cancellation fires concurrently, the awaitDone goroutine tries to acquire WLock for rows.Close(). If any prior RLock is still held, the WLock blocks, and the subsequent rows.Err() RLock blocks behind the pending WLock — classic RWMutex deadlock. The leaked DB connection never closes. The fix moves error checking to the 4 callback callers that own the rows lifecycle. Each caller now calls rows.Next() before rows.Err() to ensure locks are released and rows are fully consumed. rows.Next() returns false when iteration is done, making rows.Err() safe to call. ScanRows() (public API) is unchanged — callers already manage their own rows lifecycle per standard Go database/sql conventions. Made-with: Cursor
Member
|
why it cause deadlock? can you add some tests? |
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
2 participants
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.
Scan() calls rows.Err() after scanning, which can deadlock when a context
cancellation goroutine (awaitDone) tries to WLock the internal closemu
RWMutex concurrently. Since rows.Err() needs an RLock, it blocks behind
the pending WLock — permanent deadlock, leaked DB connection.
This moves the rows.Err() responsibility to the 4 callback callers that
actually own the rows lifecycle. Each now calls
!rows.Next()beforerows.Err(), ensuring all locks are released first. ScanRows (public API)is unchanged — callers manage their own lifecycle per standard Go patterns.
Same logical fix as #7404, rebased cleanly on current HEAD.
Made with Cursor