Introduce a new method mysqlConn.Reset() #1734
Conversation
…d on the implementation of mysqlConn.Ping(); replace the implementation of mysqlConn.Ping() with "return mc.sendNoArgsCommandWithResultOK(ctx, comPing)"
…connection (implemented as "return mc.sendNoArgsCommandWithResultOK(ctx, comConnReset)" where comConnReset=31); add tests
WalkthroughAdds three MySQL command opcodes, a reusable no-argument command helper, a public mysqlConn.Reset(ctx) method, refactors Ping to use the helper, consolidates connection tests into table-driven cases for Ping/Reset, expands driver tests for Reset behavior, and appends an AUTHORS entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MC as mysqlConn
participant Net as net.Conn
participant S as MySQL Server
rect rgb(245,250,255)
note over Client,MC: No-arg command flow (Ping / Reset)
Client->>MC: Ping(ctx) / Reset(ctx)
MC->>MC: sendSimpleCommandOK(ctx, cmd)
MC->>Net: writeCommandPacket(cmd)
Net->>S: COM_PING / COM_RESET_CONNECTION
S-->>Net: OK packet
Net-->>MC: OK packet
MC->>MC: readResultOK()
MC-->>Client: return (nil or error)
end
sequenceDiagram
autonumber
actor Test
participant DB as database/sql.Conn
participant MC as mysqlConn
participant S as MySQL Server
note over Test,MC: Reset clears server session state
Test->>S: SET @a := 1
Test->>S: SELECT @a (expect 1)
Test->>DB: Conn.Raw => mc
Test->>MC: Reset(ctx)
MC->>S: COM_RESET_CONNECTION
S-->>MC: OK
Test->>S: SELECT @a (expect NULL)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (5)
const.go (1)
118-118: Define COM_RESET_CONNECTION opcode with explicit type/hex for clarityThe value is correct (31). Minor style/readability nits:
- Make the constant explicitly typed as byte.
- Use hex to match protocol docs.
Suggested tweak:
- comConnReset = 31 + comConnReset byte = 0x1f // COM_RESET_CONNECTIONconnection.go (2)
509-525: New helper sendNoArgsCommandWithResultOK: solid; minor naming/type nitsFunctionally sound: checks closed state, wires context cancellation, clears prior results, writes the command, and reads an OK. Two small nits:
- Consider a shorter name like sendSimpleCommandOK for readability.
- No change required, but keeping the command constants typed as byte (see const.go note) avoids implicit narrowing.
No blocking issues.
688-692: Expose Reset(ctx) via COM_RESET_CONNECTION — document behavior and discoverabilityGreat addition. Two suggestions:
- Add a doc comment clarifying what is reset and server support expectations (older servers/MariaDB may return an ERR).
- Consider documenting how users can call this via database/sql.Conn.Raw with a method assertion, since mysqlConn is unexported.
Proposed comment:
-// Reset resets the MySQL connection. +// Reset resets the server-side session state using COM_RESET_CONNECTION. +// It clears most per-session state (e.g., user variables, prepared statements) +// without re-authenticating. Not all servers/versions support this command; +// in that case the server will return an error which is propagated to the caller. +// Usage hint: call via database/sql.Conn.Raw using a method assertion: +// conn.Raw(func(c any) error { +// if r, ok := c.(interface{ Reset(context.Context) error }); ok { +// return r.Reset(ctx) +// } +// return nil +// }) func (mc *mysqlConn) Reset(ctx context.Context) (err error) { return mc.sendNoArgsCommandWithResultOK(ctx, comConnReset) }driver_test.go (2)
2332-2384: TestNoArgsCommand: tighten naming/messages and close ConnThe test logic is good and validates internal state clearing. Minor cleanups:
- Subtest name currently uses method: "Conn" for Reset; make it "Reset" for consistency.
- Fix the second error message to mention insertIds (it currently repeats “affectedRows”).
- Close the acquired sql.Conn to avoid leaking it in long test runs.
Diff within this hunk:
- {method: "Conn", query: "Reset", funcToCall: func(ctx context.Context, mc *mysqlConn) error {return mc.Reset(ctx)}}, + {method: "Reset", query: "Reset", funcToCall: func(ctx context.Context, mc *mysqlConn) error {return mc.Reset(ctx)}}, @@ - conn, err := dbt.db.Conn(ctx) + conn, err := dbt.db.Conn(ctx) if err != nil { dbt.fail("db", "Conn", err) } + defer conn.Close() @@ - if got, want := c.result.insertIds, []int64(nil); !reflect.DeepEqual(got, want) { - t.Errorf("bad affectedRows: got %v, want=%v", got, want) + if got, want := c.result.insertIds, []int64(nil); !reflect.DeepEqual(got, want) { + t.Errorf("bad insertIds: got %v, want=%v", got, want) }
2386-2440: TestReset: great end-to-end verification; add resource cleanup and comment nitSolid coverage proving session variables are cleared. Two small nits:
- Close the sql.Conn after use.
- Update the nearby comment to reflect session reset rather than affectedRows/insertIds.
Suggested tweaks:
func TestReset(t *testing.T) { ctx := context.Background() runTests(t, dsn, func(dbt *DBTest) { - conn, err := dbt.db.Conn(ctx) + conn, err := dbt.db.Conn(ctx) if err != nil { dbt.fail("db", "Conn", err) } + defer conn.Close() @@ - // Check that affectedRows and insertIds are cleared after each call. + // Verify that COM_RESET_CONNECTION clears session state (e.g., user variables).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
AUTHORS(1 hunks)connection.go(3 hunks)connection_test.go(1 hunks)const.go(1 hunks)driver_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
📚 Learning: 2024-07-26T20:55:56.049Z
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
Applied to files:
connection_test.go
🧬 Code graph analysis (1)
connection_test.go (1)
dsn.go (1)
NewConfig(91-100)
🔇 Additional comments (5)
AUTHORS (1)
45-45: Add author entry — LGTMEntry is correctly placed in the alphabetized Individuals section and matches the file’s format.
connection.go (1)
505-507: Refactor Ping to reuse shared no-arg OK helper — LGTMGood consolidation. This preserves context-cancel semantics and centralizes OK-packet handling.
connection_test.go (3)
132-168: Context-cancel path for no-arg commands — LGTMAccurately asserts context.Canceled, and verifies the connection remains open and not watching. Good reuse across Ping/Reset.
171-197: ErrBadConn mapping on write failure — LGTMGood use of a fake net.Conn to exercise the errBadConnNoWrite → driver.ErrBadConn translation in the new helper, applied to both Ping and Reset.
199-225: Partial write maps to underlying error — LGTMCovers the partial-write case (n>0), ensuring the original error is propagated for both commands. Nice negative-path parity with the previous test.
|
Probably we could add the new method into the interface introduced in #1454 to allow calling it from outside easier. |
…stants (comDaemon and comBinlogDumpGTID) to make their values auto-generated
…sendSimpleCommandOK(), rename related tests accordingly
…ndOK) and TestReset: fix messages and comments, close DB connections
|
PTAL |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
const.go (1)
118-121: New COM_ opcodes align with MySQL protocol — LGTM, add tiny doc nits*
- The numeric assignments for comDaemon (29), comBinlogDumpGTID (30), and comResetConnection (31) match the protocol sequence following comStmtFetch (28). Good catch keeping the iota sequence intact.
- Nit: consider appending brief doc links/comments next to each for future maintainers.
Apply this minimal doc diff:
- comDaemon - comBinlogDumpGTID - comResetConnection + comDaemon // https://dev.mysql.com/doc/dev/mysql-server/latest/group__group__cs__com__reset__connection.html (see command list) + comBinlogDumpGTID // https://dev.mysql.com/doc/dev/mysql-server/latest/group__group__cs__com__binlog__dump__gtid.html + comResetConnection // https://dev.mysql.com/doc/dev/mysql-server/latest/group__group__cs__com__reset__connection.htmlconnection_test.go (1)
132-170: Clean-cancel path coverage — LGTM, minor clarity nitThis accurately asserts early-cancel behavior without touching the network. Nit: a short comment that startWatcher initializes internals but doesn’t flip watching until watchCancel would help future readers.
mc := &mysqlConn{ closech: make(chan struct{}), } - mc.startWatcher() + mc.startWatcher() // prepares close channel; does not enable watching until watchCancel attaches
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
connection.go(3 hunks)connection_test.go(1 hunks)const.go(1 hunks)driver_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- connection.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
🧬 Code graph analysis (1)
connection_test.go (1)
dsn.go (1)
NewConfig(91-100)
🔇 Additional comments (3)
driver_test.go (1)
2324-2331: No functional change in TestPing block end — LGTMLooks like a harmless brace/spacing churn. No action needed.
connection_test.go (2)
171-198: Bad-connection path maps I/O error to driver.ErrBadConn — LGTMThe badConnection stub and expectation are spot-on for the simple-command path.
199-226: Partial-write error propagates as original error — LGTMGood negative coverage ensuring non-zero n on Write returns the underlying error instead of ErrBadConn.
…_CONNECTION is not supported by the DB, support the text protocol when using driver.Rows() in TestReset()
|
PTAL |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
driver_test.go (1)
2392-2451: Reset test is solid; make the post-reset read robust against stale destGreat end-to-end check that COM_RESET_CONNECTION clears user variables and internal caches, with proper skip for unsupported servers. One small robustness tweak: reinitialize the destination slice before the second Next() to rule out any chance of passing or failing due to stale content if a buggy driver didn’t overwrite the slot.
rows, err = c.QueryContext(ctx, "SELECT @a", nil) if err != nil { dbt.fail("Conn", "QueryContext", err) } - err = rows.Next(result) + // reset the destination to avoid stale content affecting the assertion + result = []driver.Value{nil} + err = rows.Next(result) if err != nil { dbt.fail("Rows", "Next", err) } err = rows.Close() if err != nil { dbt.fail("Rows", "Close", err) } if !reflect.DeepEqual([]driver.Value{nil}, result) { dbt.Fatalf("Reset did not reset the session (@a is still set): got %v, want=%v", result, []driver.Value{nil}) }
🧹 Nitpick comments (2)
driver_test.go (2)
2356-2367: Prefer QueryContext and check Close() errors to harden the setup queryUse the context-aware helper consistently and assert Close() to avoid masking resource/transport errors.
- q, err := c.Query(`SELECT 1`, nil) + q, err := c.QueryContext(ctx, `SELECT 1`, nil) if err != nil { dbt.fail("Conn", "Query", err) } if got, want := c.result.affectedRows, []int64{0}; !reflect.DeepEqual(got, want) { dbt.Fatalf("bad affectedRows: got %v, want=%v", got, want) } if got, want := c.result.insertIds, []int64{0}; !reflect.DeepEqual(got, want) { dbt.Fatalf("bad insertIds: got %v, want=%v", got, want) } - q.Close() + if err := q.Close(); err != nil { + dbt.fail("Rows", "Close", err) + }
2360-2365: Assertion target uses nil slices; confirm that Reset/Ping intentionally set nil vs empty slicesThe checks assert nil (not zero-length) for affectedRows/insertIds. If the intended contract is “cleared to nil”, this is perfect; if “cleared (nil or empty)” is acceptable, broaden the assertion to accept both. Just flagging in case implementation uses empty slices.
Would you like me to adjust the assertions to accept both nil and empty slices (e.g., with a helper like isNilOrEmptyInt64Slice)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
driver_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
📚 Learning: 2024-07-26T20:55:56.049Z
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
Applied to files:
driver_test.go
🔇 Additional comments (2)
driver_test.go (2)
2330-2330: No actionable changes in this hunk.This single-line change appears to be a re-touch of the closing brace for TestPing; nothing to review here.
2369-2384: Ensure Go 1.22+ or revert numeric-range loops
We didn’t find ago.moddeclaring a minimum Go version, and CI configs don’t explicitly pin Go 1.22+. Thefor range <int>syntax is only valid on Go 1.22 or newer. Please either:
- Confirm your CI/build pipeline is using Go 1.22+.
- Or replace these loops with classic
for i := 0; i < N; i++ { … }for broader compatibility.Occurrences of
for range <int>in tests:
- connection_test.go:153
- utils.go:185
- driver_test.go:2369
- driver_test.go:2644
- driver_test.go:2751
…esult of rows closing there, add a comment explaining why we do not clear the destination before calling rows.Next() in TestReset
|
PTAL |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
driver_test.go (3)
2355-2386: Option: reseed state on each iteration to assert “clear” behavior twice, not just idempotenceRight now you seed affectedRows/insertIds once, then call Ping/Reset twice. The second iteration only re-validates that already-nil slices remain nil. If you want to assert “clear from non-nil” twice, reseed inside the loop.
Apply within this block:
- // Issue a query that sets affectedRows and insertIds. - q, err := c.QueryContext(ctx, `SELECT 1`, nil) - if err != nil { - dbt.fail("Conn", "QueryContext", err) - } - if got, want := c.result.affectedRows, []int64{0}; !reflect.DeepEqual(got, want) { - dbt.Fatalf("bad affectedRows: got %v, want=%v", got, want) - } - if got, want := c.result.insertIds, []int64{0}; !reflect.DeepEqual(got, want) { - dbt.Fatalf("bad insertIds: got %v, want=%v", got, want) - } - if err := q.Close(); err != nil { - dbt.fail("Rows", "Close", err) - } - - // Verify that Ping()/Reset() clears both fields. - for range 2 { + // Verify that Ping()/Reset() clears both fields (twice). + for range 2 { + // Reseed internal result caches to non-nil state each time. + q, err := c.QueryContext(ctx, `SELECT 1`, nil) + if err != nil { + dbt.fail("Conn", "QueryContext", err) + } + if got, want := c.result.affectedRows, []int64{0}; !reflect.DeepEqual(got, want) { + dbt.Fatalf("bad affectedRows: got %v, want=%v", got, want) + } + if got, want := c.result.insertIds, []int64{0}; !reflect.DeepEqual(got, want) { + dbt.Fatalf("bad insertIds: got %v, want=%v", got, want) + } + if err := q.Close(); err != nil { + dbt.fail("Rows", "Close", err) + } + if err := test.funcToCall(ctx, c); err != nil { // Skip Reset on servers lacking COM_RESET_CONNECTION support. if test.query == "Reset" { maybeSkip(t, err, 1047) // ER_UNKNOWN_COM_ERROR maybeSkip(t, err, 1235) // ER_NOT_SUPPORTED_YET } dbt.fail(test.method, test.query, err) } if got, want := c.result.affectedRows, []int64(nil); !reflect.DeepEqual(got, want) { t.Errorf("bad affectedRows: got %v, want=%v", got, want) } if got, want := c.result.insertIds, []int64(nil); !reflect.DeepEqual(got, want) { t.Errorf("bad insertIds: got %v, want=%v", got, want) } }
2339-2341: Future-facing: consider exposing Reset via the external interface once #1454 landsUsing Conn.Raw + type assertion is fine for tests. For external callers, once the interface from #1454 is finalized, adding Reset to that interface (or introducing a small “Resetter” extension interface) would eliminate the need for type assertions in client code.
2442-2454: Nit: use a sentinel to prove Rows.Next overwrites the destination sliceYou intentionally keep the previous non-nil value in result before the second SELECT to verify the driver writes nil. Making the sentinel explicit improves readability and makes the intent obvious to future readers.
- // We intentionally do not clear the destination before calling Next() - // to make sure the SELECT really returns nil - err = rows.Next(result) + // Seed with a sentinel to ensure Rows.Next overwrites it with nil. + result = []driver.Value{"sentinel-non-nil"} + err = rows.Next(result)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
driver_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
📚 Learning: 2024-07-26T20:55:56.049Z
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
Applied to files:
driver_test.go
🔇 Additional comments (3)
driver_test.go (3)
2330-2331: No-op formatting changeLooks like just a closing brace adjustment for TestPing. Nothing to review here.
2332-2392: Good addition: validates Ping/Reset clear internal caches and gracefully skips unsupported servers
- Table-driven subtests that exercise both Pinger.Ping and Conn.Reset via Raw are clear and focused.
- Nice touch to validate both affectedRows and insertIds become nil and to skip on ER_UNKNOWN_COM_ERROR (1047) / ER_NOT_SUPPORTED_YET (1235).
2394-2460: Robust session reset test; nice handling of protocol/value variants and server capability
- Accepting both int64(1) and []byte("1") before Reset covers text/binary protocol differences.
- maybeSkip on 1047/1235 keeps this test portable across older MySQL/MariaDB.
|
PTAL |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
driver_test.go (2)
2355-2368: Avoid potential flakiness: verify result after rows.Close, not beforeFor SELECT, the driver may finalize result metadata (affectedRows/insertIds) only after the result set is fully consumed/closed. Checking these fields before q.Close risks a race across server versions/protocols.
Move the assertions below q.Close:
- if got, want := c.result.affectedRows, []int64{0}; !reflect.DeepEqual(got, want) { - dbt.Fatalf("bad affectedRows: got %v, want=%v", got, want) - } - if got, want := c.result.insertIds, []int64{0}; !reflect.DeepEqual(got, want) { - dbt.Fatalf("bad insertIds: got %v, want=%v", got, want) - } if err := q.Close(); err != nil { dbt.fail("Rows", "Close", err) } + if got := c.result.affectedRows; len(got) != 1 || got[0] != 0 { + dbt.Fatalf("bad affectedRows: got %v, want [0]", got) + } + if got := c.result.insertIds; len(got) != 1 || got[0] != 0 { + dbt.Fatalf("bad insertIds: got %v, want [0]", got) + }
2380-2385: Prefer explicit nil checks over reflect.DeepEqual for slicesThese assertions are specifically verifying nil (not just “empty”). Use direct nil checks for clarity and to avoid reflect costs.
- if got, want := c.result.affectedRows, []int64(nil); !reflect.DeepEqual(got, want) { - t.Errorf("bad affectedRows: got %v, want=%v", got, want) - } - if got, want := c.result.insertIds, []int64(nil); !reflect.DeepEqual(got, want) { - t.Errorf("bad insertIds: got %v, want=%v", got, want) - } + if c.result.affectedRows != nil { + t.Errorf("bad affectedRows: got %v, want nil", c.result.affectedRows) + } + if c.result.insertIds != nil { + t.Errorf("bad insertIds: got %v, want nil", c.result.insertIds) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
driver_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
📚 Learning: 2024-07-26T20:55:56.049Z
Learnt from: shogo82148
PR: go-sql-driver/mysql#1576
File: CHANGELOG.md:1-6
Timestamp: 2024-07-26T20:55:56.049Z
Learning: PRs #1562 and #1570 in the go-sql-driver/mysql repository collectively address the same underlying issue related to fixing a race condition when a context is canceled, as documented in the CHANGELOG.md for version 1.8.1.
Applied to files:
driver_test.go
🔇 Additional comments (2)
driver_test.go (2)
2325-2331: TestPing change looks goodMinor cleanup, behavior unchanged. Ping path remains exercised.
2394-2455: Reset test is robust and portableGood end-to-end validation that COM_RESET_CONNECTION clears session state. Nice touches:
- Accepting both int64(1) and []byte("1") for row values.
- Seeding the second read with a non-nil sentinel to prove overwrite.
- Skipping gracefully on servers returning 1047/1235.
No changes requested.
This is something that had been done this way before the PR was created. |
Description
This PR introduces a new method mysqlConn.Reset() sending COM_RESET_CONNECTION to the connection and checking the result.
As discussed in #1273, although there are strong reasons (#1273 (comment), #1273 (comment)) to call this new method in mysqlConn.ResetSession() (probably at the end of the method), we are not going to call it for now because it has not been decided yet.
Here, we only add the implementation to allow people not to go through creating dirty hacks like those here: https://github.com/France-ioi/AlgoreaBackend/blob/master/app/database/mysql_conn_reset.go#L95
Checklist