mysql: hold the request queue refs as RefPtr<JSMySQLQuery>

Closed
RO
3mo ago
+71-112

Problem

  • The MySQL request queue (src/sql_jsc/mysql/MySQLRequestQueue.rs) holds one intrusive ref per queued JSMySQLQuery, but stored raw pointers: add() called ref_(), and each of the five removal sites paired it with an unsafe { JSMySQLQuery::deref(request) }.
  • The invariant lived in comments. The compiler could not catch a missing or a double release.

Fix

  • The element type is now the ref: type Queue = VecDeque<RefPtr<JSMySQLQuery>>. do_run passes this.ref_guard() into enqueue_request, the one place a ref is taken. Each removal is a pop_front() whose result drops, which is the release.
  • Correct because a RefPtr owns one count and releases it on Drop, so the queue holds as many refs as it has elements. Re-entrancy handling is unchanged: no borrow of the deque spans a call that can re-enter the queue.
  • LinearFifo never runs item destructors and requires T: Copy since #39570, so the container becomes a VecDeque.
  • Verified: a new test in test/js/sql/sql-mysql-clean-reentry.test.ts drives the queue release paths (five rounds of 10 queued requests rejected by clean(), then GC finalizes the connection). Also bun bd test test/js/sql/sql-mysql.test.ts (99 pass, 7 MariaDB-specific failures identical on unmodified main), 17 other sql-mysql* files, and a smoke script under ASAN (Notes). The change preserves behavior, so the new test passes on main too. It pins the release-exactly-once behavior on both sides.

Background

  • JSMySQLQuery is the Rust side of a MySQLQuery JS object, intrusively refcounted: the JS wrapper holds one ref, the queue one per element.
  • RefPtr<T> (src/ptr/ref_count.rs) is the owning handle for such a count: Drop releases, Clone takes another ref. ref_guard() returns a new one taken on &self.
  • ThisPtr<T> is a non-owning pointer to a live pointee. current_ref() returns one, so callers hold no borrow into the deque.
Notes

On tests: this is the ownership rework requested in review, not a bug fix. The manual ref/deref pairs on main are balanced, so no test can fail on main and pass here. The new test in sql-mysql-clean-reentry.test.ts pins the release-exactly-once behavior on both sides instead: it drives clean() and the GC finalizer five rounds under ASAN, where a double release crashes and a missed release hangs the round.

This PR was stacked on #37665 (OwnedRef). #40478 made RefPtr the RAII owning handle, so the PR is rebased onto main and uses RefPtr in that role. The earlier version kept LinearFifo<OwnedRef<..>> and added LinearFifo::peek_item_ref. Since #39570, LinearFifo rejects droppable element types (T: Copy on every impl), so the queue is a VecDeque now, the same move that PR made for the Valkey queues.

The file had 6 unsafe blocks. It has 0 now. cargo check, cargo clippy and rustfmt are clean for bun_sql_jsc, and cargo check --target x86_64-pc-windows-msvc -p bun_sql_jsc passes.

Smoke script (not committed), run on the debug ASAN build against the container's MariaDB 11.8, output identical to the release build of main:

  1. 600 pipelined inserts, selects and simple queries plus a transaction on one connection (add, advance releases completed heads).
  2. A SLEEP(0.2) holds the connection, then 120 queued requests of which 60 have an out-of-range BigInt bind. Their run() fails inside advance() and the run-failure path releases them (60 ERR_OUT_OF_RANGE, the rest succeed or reject at bind time).
  3. close({ timeout: 0 }) with 50 queued requests: clean() rejects all 50 with ERR_MYSQL_CONNECTION_CLOSED.
  4. A reject handler that re-enters close() while clean() drains.
  5. Ten rounds of close-with-queued-work followed by Bun.gc(true), so connections are finalized with work still queued (Drop).

The 7 failures in sql-mysql.test.ts are MariaDB vs MySQL differences (CAST(.. AS JSON), error wording, FROM_UNIXTIME(0) before the epoch, repeat() returning a blob) and two debug-build timeouts (10,000 inserts in a 10 s budget). They fail the same way on unmodified main in this container. sql-mysql.transactions.test.ts has 2 error-wording failures of the same kind.

Unrelated observation, same on the release build of main: pipelining SELEC nope (a syntax error) behind a SLEEP(0.2) on one connection hangs the queued requests. Not touched here.