Reduce "busy buffer" logs #1641
Conversation
WalkthroughThis pull request introduces several changes across three files: 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: 1
🧹 Outside diff range and nitpick comments (3)
buffer.go (2)
46-49: Fix typo in comment: "retruns" → "returns"The implementation looks good and aligns well with the PR's objective of reducing busy buffer errors. However, there's a small typo in the comment.
-// busy retruns true if the buffer contains some read data. +// busy returns true if the buffer contains some read data.
46-49: Consider documenting the synchronization guaranteesThe
busy()method is a key part of preventing buffer state issues. Consider adding documentation that explicitly states:
- The relationship between
busy()and error prevention- The synchronization guarantees that make this safe
- Usage guidelines for connection handling
This would help maintainers understand the critical role this method plays in preventing
errBadConnNoWriteerrors.connection.go (1)
128-129: Typo in function comment: 'cleare' should be 'clears'Apply this diff to correct the typo:
-// close closes the network connection and cleare results without sending COM_QUIT. +// close closes the network connection and clears results without sending COM_QUIT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
buffer.go(1 hunks)connection.go(3 hunks)packets.go(12 hunks)
🔇 Additional comments (9)
buffer.go (1)
46-49: Verify integration with connection handling
The implementation is correct, but let's verify its integration with ResetSession() and IsValid() as mentioned in the PR objectives.
✅ Verification successful
Integration with connection handling is properly implemented
The verification confirms that buffer.busy() is correctly integrated in both ResetSession() and IsValid() methods:
- In
ResetSession(): Returnsdriver.ErrBadConnif buffer is busy - In
IsValid(): Returns false if buffer is busy, indicating an invalid connection state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of buffer.busy() method in connection handling
# Check for calls to buffer.busy() in connection.go
echo "Checking buffer.busy() usage in connection handling:"
ast-grep --pattern 'if $x.busy() { $$$'
# Look for ResetSession and IsValid implementations
echo -e "\nChecking ResetSession and IsValid implementations:"
rg -A 5 'func.*(?:ResetSession|IsValid).*\{'
Length of output: 861
packets.go (5)
Line range hint 127-137: LGTM: Improved error handling in writePacket
The changes improve error handling by:
- Adding proper cancellation check
- Only returning errBadConnNoWrite for first write attempt
- Preserving original errors in other cases
This aligns with the PR objective to reduce 'busy buffer' logs while maintaining proper error handling.
309-310: LGTM: Consistent error handling
Using cleanup() instead of close() is consistent with the overall changes to improve connection handling.
400-401: LGTM: Consistent error handling
Using cleanup() instead of close() is consistent with the overall changes to improve connection handling.
996-996: LGTM: Simplified error handling
The changes appropriately simplify error handling by directly returning errors without unnecessary logging.
Also applies to: 1194-1194
Line range hint 35-76: Verify error handling flow in readPacket
The changes align with the PR objectives by:
- Using mc.close() instead of mc.Close() to avoid sending COM_QUIT
- Removing redundant error logging
- Directly returning errors instead of converting to ErrInvalidConn
However, let's verify the impact of these changes on error handling.
✅ Verification successful
Based on the results, I can now generate the final response:
Error handling changes in readPacket are safe and consistent
The changes to error handling in readPacket() are verified to be safe because:
- ErrInvalidConn is still returned in critical failure scenarios (malformed packets, connection issues)
- Test cases explicitly verify ErrInvalidConn returns in packets_test.go
- Other components (transactions, statements) handle ErrInvalidConn appropriately
- The error propagation flow remains consistent with the codebase's error handling patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other places that might expect ErrInvalidConn
rg "ErrInvalidConn" --type go -B 3 -A 3
# Check for error handling patterns
ast-grep --pattern 'if err := $_.readPacket(); err != nil {
$$$
}'
Length of output: 3235
connection.go (3)
124-126: Refactoring enhances idempotency of Close method
The call to mc.close() within the Close() method improves separation of concerns and ensures consistent cleanup without sending COM_QUIT multiple times.
644-646: Appropriate buffer busy check in ResetSession()
Adding mc.buf.busy() to the condition in ResetSession() ensures the session reset only proceeds when the buffer is not busy, preventing potential driver.ErrBadConn errors due to ongoing operations.
678-680: Enhanced validity check in IsValid() method
Including !mc.buf.busy() in the return condition accurately reflects the connection's validity by ensuring the buffer is not occupied, thereby providing a more reliable status check.
| func (mc *mysqlConn) close() { | ||
| mc.cleanup() | ||
| mc.clearResult() |
There was a problem hiding this comment.
Potential concurrency issue with mc.clearResult() in mc.close()
Calling mc.clearResult() inside mc.close() may introduce concurrency issues because mc.clearResult() is not thread-safe, and mc.close() can be invoked from multiple goroutines. Previously, the comment in mc.cleanup() mentions avoiding mc.clearResult() to prevent unsafe conditions.
Consider removing mc.clearResult() from mc.close() or ensuring it is called in a thread-safe manner to prevent race conditions.
| @@ -32,11 +32,11 @@ func (mc *mysqlConn) readPacket() ([]byte, error) { | |||
| // read packet header | |||
| data, err := mc.buf.readNext(4) | |||
| if err != nil { | |||
| mc.close() | |||
| if cerr := mc.canceled.Value(); cerr != nil { | |||
There was a problem hiding this comment.
cancelled connection was not closed. it caused busy buffer.
| @@ -45,7 +45,7 @@ func (mc *mysqlConn) readPacket() ([]byte, error) { | |||
|
|
|||
| // check packet sync [8 bit] | |||
| if data[3] != mc.sequence { | |||
| mc.Close() | |||
| mc.close() | |||
There was a problem hiding this comment.
this sends COM_QUIT during reading packet. It printed "busy buffer" in log.
| // in connection initialization we don't risk retrying non-idempotent actions. | ||
| if err == ErrInvalidConn { | ||
| return nil, "", driver.ErrBadConn | ||
| } |
There was a problem hiding this comment.
readHandshakePacket() is called during connect. And database/sql don't check ErrBadConn from connect.
methane
force-pushed
the
remove-some-error-logs
branch
from
d366da3
to
0a225ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
connection.go (1)
124-126: Consider logging COM_QUIT errorsWhile ignoring the COM_QUIT error aligns with the PR's objective, logging it could help with debugging connection issues.
Consider:
func (mc *mysqlConn) Close() (err error) { if !mc.closed.Load() { - err = mc.writeCommandPacket(comQuit) + if err = mc.writeCommandPacket(comQuit); err != nil { + mc.log("Error sending COM_QUIT:", err) + } } mc.close() return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
buffer.go(1 hunks)connection.go(3 hunks)packets.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- buffer.go
- packets.go
🔇 Additional comments (2)
connection.go (2)
644-644: LGTM: Improved session reset safety
The addition of mc.buf.busy() check alongside mc.closed.Load() effectively prevents session reset attempts when the buffer is in use, reducing the likelihood of busy buffer errors.
678-678: LGTM: Enhanced connection validity check
The addition of mc.buf.busy() check provides a more accurate representation of connection validity by considering the buffer state.
connection.go
Outdated
| // close closes the network connection and cleare results without sending COM_QUIT. | ||
| func (mc *mysqlConn) close() { | ||
| mc.cleanup() | ||
| mc.clearResult() | ||
| return | ||
| } |
There was a problem hiding this comment.
Critical: Unsafe concurrent access in close()
The close() method calls mc.clearResult() which is not thread-safe, as noted in the cleanup() method's comment: "This function can be called from multiple goroutines. So we can not mc.clearResult() here." This could lead to race conditions.
Consider this safer implementation:
func (mc *mysqlConn) close() {
mc.cleanup()
- mc.clearResult()
}If clearing the result is necessary, consider:
- Adding synchronization around
clearResult() - Documenting that
close()must only be called from a single goroutine - Moving the
clearResult()responsibility to the caller
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // close closes the network connection and cleare results without sending COM_QUIT. | |
| func (mc *mysqlConn) close() { | |
| mc.cleanup() | |
| mc.clearResult() | |
| return | |
| } | |
| // close closes the network connection and cleare results without sending COM_QUIT. | |
| func (mc *mysqlConn) close() { | |
| mc.cleanup() | |
| } |
Co-authored-by: ICHINOSE Shogo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
connection.go (1)
128-128: Fix grammar in commentThe comment has a minor grammatical error.
-// close closes the network connection and clear results without sending COM_QUIT. +// close closes the network connection and clears results without sending COM_QUIT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
buffer.go(1 hunks)connection.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buffer.go
🔇 Additional comments (2)
connection.go (2)
644-644: LGTM: Improved error handling with buffer state check
The addition of mc.buf.busy() check enhances connection state validation and aligns with the PR's objective to reduce busy buffer logs.
678-678: LGTM: Consistent connection validation
The addition of !mc.buf.busy() check maintains consistency with ResetSession and improves connection state validation.
| // close closes the network connection and clear results without sending COM_QUIT. | ||
| func (mc *mysqlConn) close() { | ||
| mc.cleanup() | ||
| mc.clearResult() |
There was a problem hiding this comment.
Critical: Thread-safety violation in close()
The close() method calls mc.clearResult() which is documented as not thread-safe in the cleanup() method's comment: "This function can be called from multiple goroutines. So we can not mc.clearResult() here." This could lead to race conditions.
Consider this safer implementation:
func (mc *mysqlConn) close() {
mc.cleanup()
- mc.clearResult()
}If clearing the result is necessary, consider one of these approaches:
- Add synchronization around
clearResult() - Document that
close()must only be called from a single goroutine - Move the
clearResult()responsibility to the caller
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // close closes the network connection and clear results without sending COM_QUIT. | |
| func (mc *mysqlConn) close() { | |
| mc.cleanup() | |
| mc.clearResult() | |
| // close closes the network connection and clear results without sending COM_QUIT. | |
| func (mc *mysqlConn) close() { | |
| mc.cleanup() | |
| } |
Description
Reduce the use of
errBadConnNoWriteto improve maintainability.ResetSession() and IsValid() checks if the buffer is busy. This reduces the risk of busy buffer error during connection in use. In principle, the risk of this is zero. So I removed errBadConnNoWrite when checking the busy buffer.
After this change, only
writePacke()returns errBadConnNoWrite.Additionally, I do not send COM_QUIT when readPacket() encounter read error.
It caused "busy buffer" error too and hide real errors.
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes