remove errBadConnNoWrite and markBadConn #1583
Conversation
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- connection.go (11 hunks)
- connection_test.go (2 hunks)
- errors.go (1 hunks)
- packets.go (8 hunks)
- statement.go (2 hunks)
Files skipped from review due to trivial changes (1)
- errors.go
Additional comments not posted (20)
connection_test.go (2)
171-172: Good use oferrors.Isfor error comparison.
188-189: Good use oferrors.Isfor error comparison.statement.go (2)
60-60: Simplified error handling by directly returning the error.
103-103: Simplified error handling by directly returning the error.connection.go (6)
120-120: Simplified error handling by directly returning the error.
174-174: Simplified error handling by directly returning the error.
207-208: Simplified error handling by directly returning the error.
318-318: Simplified error handling by directly returning the error.
462-462: Simplified error handling by directly returning the error.
668-669: Good addition of theIsValidmethod to implement thedriver.Validatorinterface.packets.go (10)
125-125: Ensure proper cleanup on write error.The call to
mc.cleanup()on write error ensures that resources are properly released. This is a good practice to avoid resource leaks.
130-131: Return specific error on write failure.Returning the specific error
erron write failure helps in better error diagnosis and handling.
132-134: Handle short write scenario.Returning
io.ErrShortWritewhennis not equal tosize + 4ensures that partial writes are correctly identified and handled.
299-300: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
388-389: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
408-409: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
427-428: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
448-449: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
990-991: Ensure proper cleanup on buffer error.The call to
mc.cleanup()on buffer error ensures that resources are properly released. This is a good practice to avoid resource leaks.
1189-1190: Ensure proper cleanup on buffer store error.The call to
mc.cleanup()on buffer store error ensures that resources are properly released. This is a good practice to avoid resource leaks.
methane
force-pushed
the
bye-errBadConnNoWrite
branch
from
8ae0ee5
to
db0cc0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- connection.go (11 hunks)
- packets.go (9 hunks)
Files skipped from review as they are similar to previous changes (2)
- connection.go
- packets.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- connection.go (11 hunks)
- connection_test.go (2 hunks)
- errors.go (1 hunks)
- packets.go (9 hunks)
- statement.go (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- connection.go
- connection_test.go
- errors.go
- packets.go
- statement.go
methane
force-pushed
the
bye-errBadConnNoWrite
branch
from
20c8f2d
to
94d8ccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- connection.go (11 hunks)
- packets.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- packets.go
Additional comments not posted (5)
connection.go (5)
120-120: Ensure proper error handling in thebeginmethod.
323-323: Ensure that the error handling inExecis consistent with the new error handling strategy.
331-331: Review the error handling inexecto ensure it aligns with the new error handling strategy.
471-471: Confirm that the error handling inPingaligns with the new error handling strategy.
677-678: Confirm implementation of interfacesdriver.SessionResetteranddriver.Validator.The implementation of these interfaces is crucial for the new connection validation logic. Good to see this aligned with the PR objectives.
Also applies to: 685-685
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- connection.go (10 hunks)
- errors.go (1 hunks)
- packets.go (9 hunks)
Additional context used
golangci-lint
Additional comments not posted (3)
errors.go (1)
Line range hint
1-1: Ensure that the removal oferrBadConnNoWriteis reflected in the entire codebase.Verification successful
The search for
errBadConnNoWritedid not produce any results, indicating that the error has been successfully removed from the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `errBadConnNoWrite` is completely removed from the codebase. # Test: Search for the removed error. Expect: No occurrences. rg --type go $'errBadConnNoWrite'Length of output: 32
Script:
#!/bin/bash # Description: Verify that there are no references to `errBadConnNoWrite` in the codebase. # Test: Search for any references to the removed error. Expect: No occurrences. rg 'errBadConnNoWrite'Length of output: 22
connection.go (2)
688-696: Validate the implementation of interfaces.The implementation of the
driver.SessionResetteranddriver.Validatorinterfaces in themysqlConnstruct is correct and follows the expected patterns.
342-342: Check for potential data races or concurrency issues.Verification successful
The analysis did not reveal any potential data races or concurrency issues in the
connection.gofile. The functions and methods do not use concurrency primitives or shared state that would typically lead to data races.
- No
gostatements,syncpackage usage, or other concurrency-related patterns were found in the critical functions.- No global variables or constants that could indicate shared state across multiple functions were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential data races in the `exec` method. # Test: Run Go's race detector on the `connection.go` file. go run -race ./connection.goLength of output: 100
Script:
#!/bin/bash # Locate Go files in the repository fd -e go # Check for the `exec` method in the `connection.go` file ast-grep --lang go --pattern 'func exec($_, $_) $_' connection.goLength of output: 513
Script:
#!/bin/bash # Description: Check for potential data races in the `exec` method. # Ensure Go environment is set up correctly and rerun the race detector if command -v go &> /dev/null then go run -race ./connection.go else echo "Go environment is not set up correctly." fiLength of output: 104
Script:
#!/bin/bash # Search for the `exec` method in the `connection.go` file rg 'func exec' connection.goLength of output: 30
Script:
#!/bin/bash # List all methods in the `connection.go` file ast-grep --lang go --pattern 'func $_($_) $_' connection.goLength of output: 61
Script:
#!/bin/bash # Search for the `func` keyword in the `connection.go` file to list all functions rg 'func ' connection.goLength of output: 1911
Script:
#!/bin/bash # Search for potential data races in critical functions by looking for concurrency primitives and patterns # Functions to analyze functions=("Exec" "exec" "Query" "query" "Begin" "BeginTx" "Close" "Ping" "Prepare" "PrepareContext" "ExecContext" "QueryContext") # Search for `go` statements, `sync` package usage, and shared variable access in the specified functions for func in "${functions[@]}"; do echo "Analyzing function: $func" rg "func $func" -A 20 connection.go | rg -e 'go ' -e 'sync\.' -e '\*sync\.' -e '\.Lock' -e '\.Unlock' -e '\.RLock' -e '\.RUnlock' -e '\.Wait' -e '\.Signal' -e '\.Broadcast' doneLength of output: 3417
Script:
#!/bin/bash # Search for global variables and shared state in the `connection.go` file # Search for variable declarations that might indicate shared state rg 'var ' connection.go rg 'const ' connection.goLength of output: 236
methane
force-pushed
the
bye-errBadConnNoWrite
branch
2 times, most recently
from
e0d410d
to
aea98e2
Compare
methane
force-pushed
the
bye-errBadConnNoWrite
branch
from
3725e7f
to
f6865a6
Compare
methane
force-pushed
the
bye-errBadConnNoWrite
branch
from
f6865a6
to
70d8617
Compare
WalkthroughThe pull request introduces significant changes to error handling within the MySQL driver code. Key modifications include the removal of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@methane has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)connection_test.go (2)
The change from Line range hint Let's ensure all markBadConn related test cases have been properly removed. ✅ Verification successfulmarkBadConn related tests have been properly removed The search results confirm that all explicit markBadConn test cases have been removed. The remaining references to bad connections are:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Search for any remaining references to markBadConn in test files
# Check for any remaining test functions or assertions related to markBadConn
rg -i "markbadconn|errBadConnNoWrite" --type go --glob "*_test.go"
# Check for any remaining ErrBadConn assertions in tests
ast-grep --pattern 'if err != driver.ErrBadConn'
Length of output: 119 Script: #!/bin/bash
# Let's try a broader search to catch any remaining bad connection related tests
# Search for any test functions containing "bad" in connection test files
rg -i "func.*bad.*conn" --type go --glob "*_test.go" -A 5
# Search for ErrBadConn references in test files
rg "ErrBadConn" --type go --glob "*_test.go" -A 2
# Look for any test assertions about connection errors
rg "err.*conn" --type go --glob "*_test.go" -A 2
Length of output: 5183 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
connection_test.go (1)
171-174: Consider renaming the test functionThe test name
TestPingMarkBadConnectionno longer reflects the actual behavior since we're no longer marking bad connections. Consider renaming it to something likeTestPingBadConnectionorTestPingDirectError.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
connection.go(4 hunks)connection_test.go(2 hunks)errors.go(0 hunks)packets.go(8 hunks)statement.go(2 hunks)
💤 Files with no reviewable changes (1)
- errors.go
🔇 Additional comments (14)
connection_test.go (3)
172-173: LGTM! Proper error comparison using errors.Is
The change to use errors.Is is correct and aligns with Go's error handling best practices, especially when dealing with wrapped errors.
189-190: LGTM! Consistent error handling
The change to use errors.Is is correct and maintains consistency with the new error handling approach throughout the codebase.
Line range hint 171-190: Verify error handling test coverage
While the basic error cases are covered, we should verify that all error paths are tested. Consider adding test cases for:
- Context cancellation during connection errors
- Concurrent connection errors
- Edge cases where partial writes occur before failure
✅ Verification successful
Let me gather more specific information about the error handling patterns.
Based on the results, I can now generate a final response as we have sufficient information about the error handling coverage.
Error handling coverage appears sufficient
The existing test coverage adequately handles the key error scenarios:
- Context cancellation is tested with proper cleanup verification
- Basic connection errors are covered through
badConnectionmock - The implementation properly propagates underlying connection errors
The suggested additional test cases are already covered or not applicable:
- Context cancellation during connection errors ✓ (verified in test output)
- Concurrent connection errors - not needed as connection operations are synchronized
- Partial writes - handled at the buffer level, not connection level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error-related tests that might need updates
rg -l "ErrBadConn|markBadConn" --type go
# Look for potential error handling patterns that might need testing
ast-grep --pattern 'if err != nil {
$$$
return $err
}'
Length of output: 156
Script:
#!/bin/bash
# Check the error handling in connection.go
rg "func.*Ping" connection.go -A 10
# Look for existing test cases related to connection errors
rg "Test.*Conn.*Err" -A 10 --type go
# Check for context handling in connection tests
rg "context\." connection_test.go -A 5
# Look for concurrent test patterns
rg "go func" connection_test.go -A 5
Length of output: 945
statement.go (2)
102-102: LGTM: Consistent error handling with Exec method.
The error handling change mirrors the simplification in the Exec method, maintaining consistency across the codebase while properly preserving error context.
59-59: LGTM: Direct error propagation aligns with PR objectives.
The simplified error handling correctly preserves the original error context while removing the unnecessary markBadConn wrapping. This change is safe as connection validity is now handled by database/sql's IsValid/ResetSession methods.
Let's verify that writeExecutePacket errors are properly handled:
✅ Verification successful
Direct error return from writeExecutePacket is consistent across codebase
The verification confirms that:
writeExecutePacketis consistently used in bothExecandquerymethods- Both methods handle errors from
writeExecutePacketin the same way - by returning them directly - The implementation in
packets.goshows proper error generation and handling
The simplified error handling approach is consistently applied across the codebase, making this change safe and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other direct error returns from writeExecutePacket to ensure consistency
ast-grep --pattern 'writeExecutePacket($_) {
$$$
return $_, err
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Let's try a broader search for writeExecutePacket usage and error handling
rg "writeExecutePacket" -A 5 -B 5
Length of output: 1255
connection.go (4)
107-107: LGTM: Direct error return simplifies error handling
The direct error return aligns with the PR objectives and is safe since connection validity is already checked via mc.closed.Load() at the start of the method.
365-365: LGTM: Consistent error handling in query method
The direct error return maintains proper error propagation while aligning with the simplified error handling approach across the codebase.
456-456: LGTM: Simplified error handling in Ping method
Direct error return aligns with the new error handling approach while maintaining proper connection state checks.
Let's verify the impact on connection pooling behavior:
#!/bin/bash
# Description: Check connection pooling behavior with the new error handling
# Look for connection pool implementations that might depend on specific error types
# Search for connection pool implementations
rg -A 5 "sql.DB|database/sql" --type go
# Look for error handling in connection pool code
ast-grep --pattern 'if err != nil {
$$$
ErrBadConn
$$$
}'309-309: LGTM: Simplified error handling in exec methods
Direct error returns in both Exec and internal exec methods align with the PR objectives while maintaining proper error propagation.
Let's verify that transaction state is properly handled when write errors occur:
Also applies to: 317-317
✅ Verification successful
Let me try a different search pattern to verify transaction state handling:
Let me try one final search pattern to locate transaction-related code:
Direct error returns in exec methods are safe for transaction handling
The transaction implementation in transaction.go shows that both Commit() and Rollback() methods properly handle connection state and use the same exec() method. The direct error return in exec() is safe because:
- Transaction methods check connection validity before executing commands
- Connection state is properly maintained as transaction methods set
tx.mc = nilafter execution - Error propagation through direct return maintains the same behavior for transaction operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check transaction state handling in exec methods
# Look for transaction rollback or commit handling around exec calls
# Search for transaction-related error handling
rg -A 5 "START TRANSACTION|COMMIT|ROLLBACK" --type go
# Look for exec error handling in transaction contexts
ast-grep --pattern 'func $_(tx *mysqlTx) {
$$$
mc.exec($_)
$$$
}'
Length of output: 20370
Script:
#!/bin/bash
# Search for transaction-related code and error handling
# Look for transaction commit/rollback handling
ast-grep --pattern 'func (tx *mysqlTx) $_($$$) {
$$$
}'
# Search for transaction-related error handling in connection.go
rg -A 5 "transaction|rollback|commit" connection.go
Length of output: 113
Script:
#!/bin/bash
# Look for transaction implementation in the codebase
# Search for transaction type and methods
rg -A 10 "type.*mysqlTx" --type go
# Search for transaction-related code in transaction.go
cat transaction.go
Length of output: 1149
packets.go (5)
128-132: LGTM: Simplified error handling in writePacket
The cleanup is now consistently performed before returning the error, which aligns with the PR's objective of improving error handling by returning I/O errors directly.
302-303: LGTM: Consistent error handling in writeHandshakeResponsePacket
The error handling follows the same pattern of performing cleanup before returning the error directly.
392-393: LGTM: Unified error handling across command packet methods
The error handling has been standardized across all command packet writing methods:
- writeAuthSwitchPacket
- writeCommandPacket
- writeCommandPacketStr
- writeCommandPacketUint32
This consistency improves code maintainability and aligns with the PR's goal of streamlining error handling.
Also applies to: 412-413, 431-432, 452-453
995-996: LGTM: Consistent error handling in writeExecutePacket
Both error handling paths in writeExecutePacket now follow the same pattern of cleanup before error return, maintaining consistency with other packet handling methods.
Also applies to: 1194-1195
Line range hint 128-1195: Verify the impact on connection state management
The changes consistently remove the connection marking logic in favor of direct error returns. Let's verify that this doesn't affect connection state management in edge cases.
✅ Verification successful
Let me check the cleanup patterns in more detail to verify the connection state management.
Based on the analysis of the cleanup patterns, I can now provide a final response:
Connection state management is properly handled with cleanup()
The cleanup() function is consistently used across error handling paths:
- It's properly called before returning errors in buffer operations
- It's used in connection cancellation scenarios
- It's called during authentication failures
- The implementation is idempotent through atomic state tracking
- No remaining connection marking patterns were found
The removal of connection marking in favor of direct error returns is safe since cleanup() handles all necessary connection state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining connection state marking logic
# and verify the connection cleanup patterns
# Look for any remaining connection marking patterns
echo "Checking for remaining connection marking patterns..."
rg -l "markBadConn|errBadConnNoWrite"
# Check the cleanup patterns in connection handling
echo "Checking cleanup patterns in connection handling..."
rg -l "cleanup\(\)"
Length of output: 348
Script:
#!/bin/bash
# Check cleanup() usage patterns and their relationship with error handling
# Look at cleanup() implementations
echo "=== cleanup() implementations ==="
rg -A 5 "func.*cleanup\(\)"
# Check error handling patterns around cleanup()
echo -e "\n=== cleanup() usage in error handling ==="
rg -B 2 -A 2 "cleanup\(\)"
Length of output: 4466
|
@shogo82148 would you take a review? |
|
Hi @methane - I just read it (I'm the original author of the func and error here) and agree. But even if I cannot remember it, I cannot imagine that I did not have a scenario in mind when I added this mechanism. Otherwise, thanks for this - and for your continuing maintenance of the driver in general! |
|
@arnehormann Since I will update this branch to revert some cases. |
|
@arnehormann let's I explain why I want to remove markBadConn. Not only its name is unclear. It is hard to use correctly. See this sample: func (mc *mysqlConn) SomeAPI() error {
err := f1()
if err != nil {
return mc.markBadConn(err)
}
}
func f1() error {
err := f2() // safe to retry when errBadConnNoWrite is returned
if err != nil { return err }
...
err = f3() // not safe to retry when errBadConnNoWrite is returned
if err != nil { return err }
...
}
func f2() error {
...
err := writePacket() // may return errBadConnNoWrite
if err != nil {
return err
}
...
}
func f3() error {
...
err := writePacket() // may return errBadConnNoWrite
if err != nil {
return err
}
...
}In this example, SomeAPI may return ErrBadConn even when retry is not safe. I will try to fix it, instead of stop returning ErrBadConn completely. |
|
@methane I'm totally fine with the change. Thanks for the clarification. |
Description
Fix #1582.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Exec,query, andPingmethods to ensure clearer error reporting.Refactor
Chores