fix PING on compressed connections #1721
Conversation
WalkthroughThe changes unify and correct the handling of packet sequence numbers in both compressed and uncompressed MySQL protocol packet operations. Sequence number tracking is now consistent, with improved logging and synchronization after packet reads and writes, especially for compression scenarios. Debug output formatting is also improved. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant mysqlConn
participant compIO
participant MySQLServer
App->>mysqlConn: Ping()
mysqlConn->>compIO: writeCompressedPacket()
compIO->>MySQLServer: Send compressed packet (with compressSequence)
MySQLServer-->>compIO: Respond with compressed packet (with compressSequence)
compIO->>mysqlConn: readCompressedPacket()
mysqlConn->>App: Ping result
App->>mysqlConn: QueryRow()
mysqlConn->>compIO: writeCompressedPacket()
compIO->>MySQLServer: Send compressed packet (with compressSequence)
MySQLServer-->>compIO: Respond with compressed packet (with compressSequence)
compIO->>mysqlConn: readCompressedPacket()
mysqlConn->>App: Query result
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Pull Request Overview
This pull request addresses the "fix PING on compressed connections" issue by adding missing mc.syncSequence() calls and adjusting packet sequence handling for both compressed and uncompressed connections. The key changes include:
- Merging sequence synchronization logic in readPacket.
- Inserting mc.syncSequence() calls after writing command packets.
- Updating debug logging to improve clarity and correct sequence comparisons in both packets.go and compress.go.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packets.go | Adjusted sequence handling, added mc.syncSequence() calls, and improved logging in writePacket and writeCommandPacket functions. |
| compress.go | Corrected the debug check to compare against mc.compressSequence and refined logging in writeCompressedPacket. |
Comments suppressed due to low confidence (3)
packets.go:67
- For compressed connections, the original logic updated mc.compressSequence separately. Since compress.go relies on mc.compressSequence for debug checks, please verify that not updating it in readPacket (and instead always updating mc.sequence) does not introduce inconsistencies for compressed flows.
if seq != mc.sequence {
packets.go:1032
- [nitpick] Resetting the packet sequence immediately after sending a COM_LONG_DATA packet changes the previous reset behavior; please confirm that this aligns with the protocol requirements for handling long data packets.
stmt.mc.resetSequence()
compress.go:116
- The debug check now correctly compares against c.mc.compressSequence; however, ensure that the synchronization of this field is maintained across all packet reads to avoid potential mismatches.
if debug && compressionSequence != c.mc.compressSequence {
| @@ -146,7 +141,7 @@ func (mc *mysqlConn) writePacket(data []byte) error { | |||
|
|
|||
| // Write packet | |||
| if debug { | |||
| fmt.Printf("writePacket: size=%v seq=%v", size, mc.sequence) | |||
| fmt.Fprintf(os.Stderr, "writePacket: size=%v seq=%v\n", size, mc.sequence) | |||
There was a problem hiding this comment.
[nitpick] Consider using a centralized logging mechanism instead of direct writes to os.Stderr to ensure consistency across the project.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
this is only for debugging. debug is const and its value is false.
![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
🧹 Nitpick comments (5)
compress.go (3)
108-111: Debug log references the wrong sequence counterAfter switching to
compressSequence, the debug message still printsc.mc.sequence, which is now unrelated to the compressed-frame sequence. This will confuse anyone hunting sync issues.- fmt.Printf("uncompress cmplen=%v uncomplen=%v pkt_cmp_seq=%v expected_cmp_seq=%v\n", - comprLength, uncompressedLength, compressionSequence, c.mc.sequence) + fmt.Printf("uncompress cmplen=%v uncomplen=%v pkt_cmp_seq=%v expected_cmp_seq=%v\n", + comprLength, uncompressedLength, compressionSequence, c.mc.compressSequence)
116-121: Missing newline in warning + minor typo
- The warning lacks a trailing
\n, so it sticks to the next log line.- “cmpress” is misspelled.
- fmt.Printf("WARN: unexpected cmpress seq nr: expected %v, got %v", + fmt.Printf("WARN: unexpected compress seq nr: expected %v, got %v\n",
200-204: Stick to one stream for debug outputOther debug prints were just redirected to
os.Stderr; this one is still using the defaultStdout. Redirect it as well for uniform logging and to avoid polluting normal output.- fmt.Printf( + fmt.Fprintf(os.Stderr,packets.go (2)
448-451: CallsyncSequence()only on successful writeIf
writePacketfails we still invokesyncSequence(), which may desynchronise counters for a connection that is about to be discarded anyway.-err = mc.writePacket(data) -mc.syncSequence() -return err +if err := mc.writePacket(data); err != nil { + return err +} +mc.syncSequence() +return nil
491-494: Same conditional-call issue as aboveApply the same guard around
syncSequence()for the uint32 variant to avoid post-error side-effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compress.go(2 hunks)packets.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packets.go (1)
errors.go (1)
ErrPktSyncMul(29-29)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (windows-latest, 1.24, 9.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (ubuntu-latest, 1.22, 9.0)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, 8.4)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (macos-latest, 1.24, mariadb-11.2)
- GitHub Check: test (macos-latest, 1.24, 8.0)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.11)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, 8.4)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
packets.go (1)
66-80: Sequence-mismatch silently ignored on compressed links
invalidSequenceis no longer set whenmc.compress == true.
A real out-of-sync condition on the payload sequence will now be tolerated silently on compressed connections, potentially masking corruption.Please double-check the MySQL spec and confirm this is really safe; if not, keep the old behaviour or add a comment explaining why it’s harmless.
Add missing mc.syncSequence() Fix go-sql-driver#1718
Description
Add missing mc.syncSequence()
Fix #1718
Checklist
Summary by CodeRabbit
Bug Fixes
Style