-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove errBadConnNoWrite and markBadConn #1583
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes primarily involve refactoring error handling and logging within the Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Is
for error comparison.
188-189
: Good use oferrors.Is
for 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 theIsValid
method to implement thedriver.Validator
interface.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
err
on write failure helps in better error diagnosis and handling.
132-134
: Handle short write scenario.Returning
io.ErrShortWrite
whenn
is not equal tosize + 4
ensures 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.
8ae0ee5
to
db0cc0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
20c8f2d
to
94d8ccd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thebegin
method.
323-323
: Ensure that the error handling inExec
is consistent with the new error handling strategy.
331-331
: Review the error handling inexec
to ensure it aligns with the new error handling strategy.
471-471
: Confirm that the error handling inPing
aligns with the new error handling strategy.
677-678
: Confirm implementation of interfacesdriver.SessionResetter
anddriver.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
connection.go
Outdated
mc.cleanup() | ||
// interpolateParams would be called before sending any query. | ||
// So its safe to retry. | ||
mc.log(err) | ||
return "", ErrInvalidConn | ||
return "", driver.ErrBadConn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider simplifying the error handling in interpolateParams
.
- mc.cleanup()
- mc.log(err)
+ return "", err
This change would make the error handling more straightforward and consistent with other parts of the code.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
mc.cleanup() | |
// interpolateParams would be called before sending any query. | |
// So its safe to retry. | |
mc.log(err) | |
return "", ErrInvalidConn | |
return "", driver.ErrBadConn | |
// interpolateParams would be called before sending any query. | |
// So its safe to retry. | |
return "", err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simply a bug fix. Since it did not close the connection and also did not return ErrBadConn, if the connection was in a state where takeCompleteBuffer() failed for any reason, ErrInvalidConn could be returned repeatedly, depending on how the application was used.
This fix may need to be backported. I will create a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@methane, thank you for your insights and for highlighting the importance of this bug fix. Ensuring that ErrBadConn
is returned correctly is crucial for the robustness of error handling in connection management. If there's anything more you need regarding the backporting process or any other assistance, please feel free to let me know. Looking forward to seeing the improvements in the upcoming PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will split this PR to smaller PRs.
return err | ||
} | ||
return driver.ErrBadConn | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function and errBadConnNoWrite were completely meaningless. There is no need to use errBadConnNoWrite once and convert it to ErrBadConn, as you can just use ErrBadConn directly if you are not sending anything yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind.
markBadConn()
is allowlist for ErrBadConn.
If writePacket() returned driver.ErrBadConn instead of errBadConnNoWrite, all error check need to suppress ErrBadConn.
So until we stop returning ErrBadConn, markBadConn() should alive.
// current
if err := f1(); err != nil {
return nil, mc.markBadConn(err)
}
if err := f2(); err != nil {
return nil, err
}
if err := f3(); err != nil {
return nil, err
}
...
// if we removed markBadConn()
if err := f1(); err != nil {
return nil, err
}
if err := f2(); err != nil {
// not safe to retry
if err == errBadConnNoWrite {
err = InvalidConn
}
return nil, err
}
if err := f3(); err != nil {
if err == errBadConnNoWrite {
err = InvalidConn
}
return nil, err
}
...
connection.go
Outdated
func (mc *mysqlConn) Begin() (driver.Tx, error) { | ||
return mc.begin(false) | ||
} | ||
|
||
func (mc *mysqlConn) begin(readOnly bool) (driver.Tx, error) { | ||
if mc.closed.Load() { | ||
mc.log(ErrInvalidConn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log only confuses users and does not help. When a connection is closed, the reason should be logged if necessary.
connection.go
Outdated
mc.cleanup() | ||
// interpolateParams would be called before sending any query. | ||
// So its safe to retry. | ||
mc.log(err) | ||
return "", ErrInvalidConn | ||
return "", driver.ErrBadConn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simply a bug fix. Since it did not close the connection and also did not return ErrBadConn, if the connection was in a state where takeCompleteBuffer() failed for any reason, ErrInvalidConn could be returned repeatedly, depending on how the application was used.
This fix may need to be backported. I will create a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 oferrBadConnNoWrite
is reflected in the entire codebase.Verification successful
The search for
errBadConnNoWrite
did 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.SessionResetter
anddriver.Validator
interfaces in themysqlConn
struct 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.go
file. The functions and methods do not use concurrency primitives or shared state that would typically lead to data races.
- No
go
statements,sync
package 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
@@ -344,15 +331,15 @@ func (mc *mysqlConn) Exec(query string, args []driver.Value) (driver.Result, err | |||
copied := mc.result | |||
return &copied, err | |||
} | |||
return nil, mc.markBadConn(err) | |||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review the error handling in the Exec
method.
The error handling in the Exec
method should be consistent and clear. Consider adding more specific error messages or handling for different error scenarios.
@@ -140,7 +129,7 @@ | |||
if err == nil { | |||
return &mysqlTx{mc}, err | |||
} | |||
return nil, mc.markBadConn(err) | |||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper error handling and propagation.
The error handling in the begin
method should be consistent and clear. Consider adding more specific error messages or handling for different error scenarios.
packets.go
Outdated
if err != nil { | ||
mc.cleanup() | ||
mc.log(ErrMalformPkt) | ||
} else { | ||
if cerr := mc.canceled.Value(); cerr != nil { | ||
return cerr | ||
} | ||
if n == 0 && pktLen == len(data)-4 { | ||
// only for the first loop iteration when nothing was written yet | ||
return errBadConnNoWrite | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling specific I/O errors more gracefully.
The current implementation returns the raw error directly to the caller. It might be beneficial to classify certain types of I/O errors and handle them differently, such as temporary network errors which could be retried. This would make the error handling more robust and potentially reduce connection failures due to transient issues.
packets.go
Outdated
// io.Writer(b) must return a non-nil error if it cannot write len(b) bytes. | ||
// The io.ErrShortWrite error is used to indicate that this rule has not been followed. | ||
mc.cleanup() | ||
mc.log(err) | ||
return io.ErrShortWrite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper error handling when io.ErrShortWrite
occurs.
The current implementation returns io.ErrShortWrite
directly. It might be useful to add a log statement before returning the error for better debugging and operational visibility.
mc.cleanup() | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor repeated error handling patterns.
There is a repeated pattern where a buffer acquisition fails and mc.cleanup()
is called before returning the error. This pattern could be abstracted into a helper method to reduce code duplication and improve maintainability.
- mc.cleanup()
- return err
+ return handleBufferAcquisitionError(mc, err)
And then define the helper function:
func handleBufferAcquisitionError(mc *mysqlConn, err error) error {
mc.cleanup()
return err
}
Also applies to: 391-392, 411-412, 430-431, 451-452
packets.go
Outdated
// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_query_response.html | ||
func (mc *okHandler) readResultSetHeaderPacket() (int, error) { | ||
// handleOkPacket replaces both values; other cases leave the values unchanged. | ||
mc.result.affectedRows = append(mc.result.affectedRows, 0) | ||
mc.result.insertIds = append(mc.result.insertIds, 0) | ||
|
||
data, err := mc.conn().readPacket() | ||
if err == nil { | ||
switch data[0] { | ||
|
||
case iOK: | ||
return 0, mc.handleOkPacket(data) | ||
if err != nil { | ||
return 0, err | ||
} | ||
|
||
case iERR: | ||
return 0, mc.conn().handleErrorPacket(data) | ||
switch data[0] { | ||
case iOK: | ||
return 0, mc.handleOkPacket(data) | ||
|
||
case iLocalInFile: | ||
return 0, mc.handleInFileRequest(string(data[1:])) | ||
} | ||
case iERR: | ||
return 0, mc.conn().handleErrorPacket(data) | ||
|
||
// column count | ||
num, _, _ := readLengthEncodedInteger(data) | ||
// ignore remaining data in the packet. see #1478. | ||
return int(num), nil | ||
case iLocalInFile: | ||
return 0, mc.handleInFileRequest(string(data[1:])) | ||
} | ||
return 0, err | ||
|
||
// column count | ||
// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_query_response_text_resultset.html | ||
num, _, _ := readLengthEncodedInteger(data) | ||
// ignore remaining data in the packet. see #1478. | ||
return int(num), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimize the handling of result set headers.
The method readResultSetHeaderPacket
could be optimized by directly returning the error from handleOkPacket
and handleErrorPacket
without the intermediate assignment to data
and err
. This would simplify the control flow and potentially improve performance by reducing unnecessary variable assignments.
- data, err := mc.conn().readPacket()
- if err != nil {
- return 0, err
- }
- switch data[0] {
- case iOK:
- return 0, mc.handleOkPacket(data)
- case iERR:
- return 0, mc.conn().handleErrorPacket(data)
- case iLocalInFile:
- return 0, mc.handleInFileRequest(string(data[1:]))
- }
+ data, err := mc.conn().readPacket()
+ if err != nil {
+ return 0, err
+ }
+ return 0, mc.handlePacketBasedOnType(data)
And then define the helper function:
func (mc *okHandler) handlePacketBasedOnType(data []byte) error {
switch data[0] {
case iOK:
return mc.handleOkPacket(data)
case iERR:
return mc.conn().handleErrorPacket(data)
case iLocalInFile:
return mc.handleInFileRequest(string(data[1:]))
default:
return fmt.Errorf("unknown packet type: %v", data[0])
}
}
mc.cleanup() | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle buffer acquisition failures more gracefully.
When buffer acquisition fails, the method writeExecutePacket
simply cleans up and returns the error. Consider implementing a retry mechanism or a more informative error message to help diagnose issues related to buffer allocation failures.
30b95e8
to
e0d410d
Compare
e0d410d
to
aea98e2
Compare
Description
Fix #1582.
Checklist
Summary by CodeRabbit
New Features
IsValid
method to enhance connection validation.SessionResetter
andValidator
interfaces for improved session management.Bug Fixes
Refactor
Chores