fix some write error handling #1595
Conversation
WalkthroughThe recent updates enhance error handling in the Changes
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 (
|
| @@ -117,6 +117,8 @@ func (mc *mysqlConn) writePacket(data []byte) error { | |||
| // Write packet | |||
| if mc.writeTimeout > 0 { | |||
| if err := mc.netConn.SetWriteDeadline(time.Now().Add(mc.writeTimeout)); err != nil { | |||
| mc.cleanup() | |||
| mc.log(err) | |||
There was a problem hiding this comment.
This also reduces risk of repeated error. Although SetWriteDeadline() won't fail in real world.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1 hunks)
- packets.go (1 hunks)
Additional context used
golangci-lint
connection.go
27-27: undefined: buffer (typecheck)
30-30: undefined: mysqlResult (typecheck)
31-31: undefined: Config (typecheck)
32-32: undefined: connector (typecheck)
36-36: undefined: clientFlag (typecheck)
37-37: undefined: statusFlag (typecheck)
46-46: undefined: atomicError (typecheck)
383-383: undefined: textRows (typecheck)
577-577: undefined: mysqlStmt (typecheck)
596-596: undefined: mysqlStmt (typecheck)
118-118: undefined: errBadConnNoWrite (typecheck)
130-130: undefined: ErrInvalidConn (typecheck)
141-141: undefined: mysqlTx (typecheck)
149-149: mc.writeCommandPacket undefined (type *mysqlConn has no field or method writeCommandPacket) (typecheck)
153-153: mc.clearResult undefined (type *mysqlConn has no field or method clearResult) (typecheck)
185-185: undefined: ErrInvalidConn (typecheck)
192-192: undefined: ErrInvalidConn (typecheck)
196-196: mc.writeCommandPacketStr undefined (type *mysqlConn has no field or method writeCommandPacketStr) (typecheck)
203-203: undefined: mysqlStmt (typecheck)
211-211: mc.readUntilEOF undefined (type *mysqlConn has no field or method readUntilEOF) (typecheck)
217-217: mc.readUntilEOF undefined (type *mysqlConn has no field or method readUntilEOF) (typecheck)
277-277: undefined: appendDateTime (typecheck)
285-285: undefined: statusNoBackslashEscapes (typecheck)
286-286: undefined: escapeBytesBackslash (typecheck)
288-288: undefined: escapeBytesQuotes (typecheck)
296-296: undefined: statusNoBackslashEscapes (typecheck)
297-297: undefined: escapeBytesBackslash (typecheck)
299-299: undefined: escapeBytesQuotes (typecheck)
305-305: undefined: statusNoBackslashEscapes (typecheck)
306-306: undefined: escapeStringBackslash (typecheck)
308-308: undefined: escapeStringQuotes (typecheck)
352-352: mc.clearResult undefined (type *mysqlConn has no field or method clearResult) (typecheck)
354-354: mc.writeCommandPacketStr undefined (type *mysqlConn has no field or method writeCommandPacketStr) (typecheck)
366-366: mc.readUntilEOF undefined (type *mysqlConn has no field or method readUntilEOF) (typecheck)
384-384: mc.clearResult undefined (type *mysqlConn has no field or method clearResult) (typecheck)
402-402: mc.writeCommandPacketStr undefined (type *mysqlConn has no field or method writeCommandPacketStr) (typecheck)
408-408: undefined: textRows (typecheck)
423-423: mc.readColumns undefined (type *mysqlConn has no field or method readColumns) (typecheck)
442-442: undefined: textRows (typecheck)
444-444: undefined: mysqlField (typecheck)
492-492: mc.writeCommandPacket undefined (type *mysqlConn has no field or method writeCommandPacket) (typecheck)
511-511: undefined: mapIsolationLevel (typecheck)
525-525: undefined: namedValueToValue (typecheck)
544-544: undefined: namedValueToValue (typecheck)
578-578: undefined: namedValueToValue (typecheck)
661-661: undefined: converter (typecheck)
688-688: undefined: connCheck (typecheck)
Additional comments not posted (1)
connection.go (1)
233-236: The changes in theinterpolateParamsmethod are appropriate and align with the PR's objectives to handle connection errors more effectively.It's recommended to add a comment explaining why
mc.cleanup()is called anddriver.ErrBadConnis returned instead ofErrInvalidConn, to aid future maintainability.
| mc.cleanup() | ||
| mc.log(err) |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that ErrInvalidConn is still widely used across multiple files. To maintain consistency in error handling, consider updating all occurrences of ErrInvalidConn to driver.ErrBadConn.
packets.goconnection.gostatement.gotransaction.gopackets_test.goerrors.goconnection_test.go
Analysis chain
Proper cleanup and logging before returning errors enhance robustness.
However, consider updating the error returned from ErrInvalidConn to driver.ErrBadConn to align with the changes in connection.go as mentioned in the PR description. This ensures consistency across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if `ErrInvalidConn` is still used elsewhere in error handling.
# Test: Search for `ErrInvalidConn` usage. Expect: No occurrences except in deprecated or commented sections.
rg --type go 'ErrInvalidConn'
Length of output: 1253
interpolateParams() returned ErrInvalidConn without closing the connection. Since database/sql doesn't understand ErrInvalidConn, there is a risk that database/sql reuse this connection and ErrInvalidConn is returned repeatedly.
interpolateParams() returned ErrInvalidConn without closing the connection. Since database/sql doesn't understand ErrInvalidConn, there is a risk that database/sql reuse this connection and ErrInvalidConn is returned repeatedly. This PR is backport of #1595.
Description
interpolateParams() returned ErrInvalidConn without closing the connection.
Since database/sql doesn't understand ErrInvalidConn, there is a risk that database/sql reuse this connection and ErrInvalidConn is returned repeatedly.
This PR should be backported to 1.8.
Checklist
Summary by CodeRabbit