stmt.Close() returns nil when double close #1642
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (1)
statement.go (1)
27-31: Consider enhancing the comment clarity.While the comment explains the historical context well, consider making it more explicit about the current behavior:
// driver.Stmt.Close could be called more than once, thus this function // had to be idempotent. See also Issue #450 and golang/go#16019. // This bug has been fixed in Go 1.8. // https://github.com/golang/go/commit/90b8a0ca2d0b565c7c7199ffcf77b15ea6b6db3a -// But we keep this function idempotent because it is safer. +// We maintain idempotency by always returning nil on already-closed statements +// as this is safer and prevents unnecessary error handling complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
statement.go(1 hunks)
🔇 Additional comments (2)
statement.go (2)
27-32: LGTM! Safer handling of closed connections.
The change to return nil instead of driver.ErrBadConn when the connection is already closed is a good improvement. This makes the Close() method truly idempotent and safer for retry operations, which aligns well with the PR's objective of more careful ErrBadConn handling.
26-33: Verify error handling patterns.
Let's verify that this change aligns with error handling patterns elsewhere in the codebase.
✅ Verification successful
Let me analyze the Close() implementations and nil return patterns to verify the error handling consistency.
Error handling pattern is consistent with codebase practices
The idempotent nil return in Close() when the connection is already closed aligns perfectly with other implementations in the codebase. This is evidenced by:
- Similar nil-return patterns in other Close() implementations (mockConn, dummyConnection)
- The pattern matches the documented behavior regarding multiple Close() calls
- The error handling follows the codebase's convention of returning nil for already-closed resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error handling patterns with ErrBadConn
rg "ErrBadConn" --type go
# Look for other Close() implementations for consistency
ast-grep --pattern 'func $_($_) Close() error {
$$$
}'
Length of output: 1987
Script:
#!/bin/bash
# Look for Close() method implementations with a more specific pattern
ast-grep --pattern 'func ($_ *$_) Close() error {
$$$
}'
# Search for nil return patterns in Close methods
rg "func.*Close.*error.*\{[\s\S]*?return nil" --type go -A 5
Length of output: 1164
Description
ErrBadConn needs special care to ensure it is safe to retry.
To improve maintenance, I don't want to use the error where I don't have to.
Additionally, update the old comment about Go's bug that had been fixed long time ago.
Checklist
Summary by CodeRabbit
Bug Fixes
Documentation
Closemethod and its behavior in relation to connection handling.