remove errBadConnNoWrite and markBadConn #1583
Conversation
WalkthroughThe changes streamline error handling in the MySQL connection implementation by removing redundant error marking and logging, and directly returning errors. This includes eliminating the Changes
Assessment against linked issues
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 (
|
![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
| 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.
Consider simplifying the error handling in interpolateParams.
- mc.cleanup()
- mc.log(err)
+ return "", errThis 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.
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.
@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!
| return err | ||
| } | ||
| return driver.ErrBadConn | ||
| } |
There was a problem hiding this comment.
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.
| 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.
This log only confuses users and does not help. When a connection is closed, the reason should be logged if necessary.
| 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.
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.
Description
Fix #1582.
Checklist