Repository navigation
Conversation
Database#busy_timeout documents that its wait holds the GVL. SQLite sleeps while holding the connection's mutex, so a sleep that released the GVL would let another thread enter the connection and block on that mutex with the GVL held, and a sleep that checked for interrupts would let a Thread#raise unwind through SQLite and leave the mutex locked. - A busy_timeout wait doesn't let another thread run. - A thread using a connection while another thread waits on it doesn't deadlock (in a child process, since a deadlock stops every thread). - A Thread#raise aimed at a waiting thread leaves the connection usable from another thread (also in a child process). The child processes use spawn, not fork, so these also run on Windows, where SQLite's Sleep resolves to the Ruby DLL's rb_w32_Sleep (upstream issue 766). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SQLite's Windows VFS sleeps through the "Sleep" entry of its system-call table (winLock's lock retries, winRetryIoerr, and the default busy handler behind Database#busy_timeout), while holding the connection's mutex. Linked into this extension, that "Sleep" resolves against the Ruby DLL, which exports a "Sleep" forwarding to rb_w32_Sleep (win32/mkexports.rb turns the "#define Sleep" in include/ruby/win32.h into an export). rb_w32_Sleep releases the GVL and checks for interrupts, so another Ruby thread could enter the connection and block on its mutex with the GVL held, or a Thread#raise could unwind through SQLite and leave the mutex locked. Either deadlocks the process. At load time, look up kernel32's Sleep with GetProcAddress (naming Sleep in the source would get the ruby/win32.h macro and the same import) and install it with each Windows VFS's xSetSystemCall. Harmless when SQLite is a separate DLL, whose Sleep already comes from kernel32. Fixes upstream issue 766 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MahtraDR
left a comment
There was a problem hiding this comment.
PR Review: fix: point SQLite's Sleep at kernel32 on Windows
Reviewer: Claude · 5bd933d · 2026-11-06
The fix itself holds up, and I couldn't break it. The shipped 2.10.2 x64-mingw-ucrt gem really does import Sleep from x64-ucrt-ruby*.dll for all four Ruby versions. Every sleep in SQLite 3.54.1's Windows VFS goes through the one aSyscall "Sleep" slot this replaces: winLock, winRetryIoerr, winClose, and winSleep (which the busy handler and WAL retries call). Your tests-only and with-fix CI runs show exactly what the description says.
I also checked the tests a second way. On macOS I interposed SQLite's nanosleep with a shim that releases the GVL and checks interrupts, the way rb_w32_Sleep does. With the shim, all three tests fail every time (20 out of 20). Test 2 catches the GVL release and test 3 catches the interrupt. Without it they pass every time, and test 1 counted 0 ticks even under heavy CPU load and GC stress.
Everything left to fix is in the text that ships with the PR.
1. The CHANGELOG points shared-connection users at a method that deadlocks for them (CHANGELOG.md:15). The new "Changed" line says to use busy_handler_timeout= "for lock waits that let other threads run". That method installs a Ruby busy handler, and SQLite runs it while holding the connection's mutex. If another thread uses the same connection during the wait, it blocks on that mutex while holding the GVL, and the process hangs. That is the deadlock this PR fixes, on every platform.
I reproduced it 3 out of 3 times on macOS, against a busy_timeout control that passes every time. A handler with no sleep at all hangs too, so removing the sleep(0.001) wouldn't help. README says sharing a Database between threads is safe, and lich-5 does exactly that. Please qualify the sentence. For example: other threads can run during the wait only if they don't use the waiting connection. If threads share a connection, keep busy_timeout, or give each thread its own connection.
2. The "Changed" entry understates what Windows users will notice (CHANGELOG.md:15). It says other threads don't run during the wait. That's accurate, but it leaves out three consequences people will hit after upgrading:
- With two connections in one process (Sequel's pool, which calls
busy_timeout(5000)by default, or lich-5), the thread holding the lock can't run to commit. So the wait now always ends inBusyExceptionafter the full timeout. On macOS: 2.16 s BusyException withbusy_timeout, against success in 0.21 s withbusy_handler_timeout=. Timeout.timeout,Thread#raise/#killand Ctrl-C now take effect only when the wait ends. ATimeoutshorter than the busy timeout returnsBusyExceptioninstead ofTimeout::Error. On macOS: 2.10 s against 0.20 s.- The Windows VFS's own retries now hold the GVL too, and
busy_handler_timeout=doesn't change them. On sharing or lock violations (antivirus, indexers), winRetryIoerr sleeps up to about 1.4 s per failing call, and winClose up to 200 ms.
The change itself is intended and matches Linux and macOS. I'm only asking for a sentence or two so upgraders know what to expect, or a note from you if you'd rather not.
3. Scope (CHANGELOG.md:7 and :15). Both entries say "On Windows". Your own comment in sqlite3.c and the CI table show that only builds with SQLite linked into the extension change, which means the precompiled gems and the default build. --enable-system-libraries, mswin and SQLCipher builds already used kernel32's Sleep. Something like "On Windows, when SQLite is linked into the extension (the precompiled gems and the default build)" would cover it.
4. "As documented" (test/test_sqlite_sleep.rb:9). The test header says Database#busy_timeout documents that its sleeps don't release the GVL or check for interrupts. Its rdoc (ext/sqlite3/database.c:779) says neither. Only the busy_handler_timeout= rdoc mentions the GVL, and no doc mentions interrupts. Adding one sentence to the busy_timeout rdoc about the GVL and interrupts would make both "as documented" claims true, and would give item 2 a lasting home.
5. C comment (ext/sqlite3/sqlite3.c:219). The reason given for GetProcAddress ("Sleep is the macro from ruby/win32.h") doesn't hold. The macro is function-like, so a bare Sleep isn't expanded. The real reason is the second half of the sentence: a direct reference would bind to the same Ruby-DLL import. The list of sleep sites at :209 also leaves out winClose's retry and the WAL read retry. Both are covered by the fix.
6. The test-only hook (ext/sqlite3/sqlite3.c:243). sqlite3.h describes xSetSystemCall as a testing aid whose overridable calls can change from one release to the next, and the return code is ignored here. Your Windows CI test would catch a renamed slot, so the risk is small. But a user-built library outside CI would lose the fix silently, and maintainers may ask why not fix the link order instead. I'd suggest three things:
- make "no VFS accepted it" visible, at least in tests or debug builds;
- say in the comment that the hook is test-only and which CI test guards it;
- copy the alternatives discussion from sparklemotion#766 into the PR description.
Before you open it upstream: both CHANGELOG entries cite only issue sparklemotion#766. The house style cites the PR too ("sparklemotion#766, #NNN").
A related note on code this PR doesn't touch, for whoever looks at item 1 upstream: any Ruby callback that runs while SQLite holds the connection's mutex hangs a second thread that uses the connection, on every platform. That includes busy_handler (with or without sleep) and a create_function block that sleeps or loops. That is in some tension with README's thread-safety paragraph, but it isn't this PR's to fix.
The one red CI job, sqlcipher (windows, mingw), fails the same way on upstream main (pacman can't find the package).
…n if it can't be installed Review follow-up: - CHANGELOG: limit both entries to builds with SQLite linked into the extension (the precompiled gems and the default build); list what upgraders will notice (same-process lock holders can't run, so the wait ends in BusyException; interrupts and Timeout take effect only when the wait ends; the VFS's own sharing-violation retries hold the GVL too); and qualify the busy_handler_timeout= advice: it lets other threads run only if they don't use the waiting connection. - Database#busy_timeout rdoc: state that the wait holds the GVL and defers interrupts. Database#busy_handler_timeout= rdoc: add the same shared-connection caveat. - sqlite3.c: give the real reason for GetProcAddress (a direct reference would bind to the Ruby-DLL import; the function-like macro isn't what matters), list winClose's retry and the WAL read retry, note that xSetSystemCall is a testing aid and which test guards the fix, and warn at load time if no VFS accepts the override instead of failing silently. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@MahtraDR thanks. All six items are addressed in 15e4a37. 1. 2. Upgrade effects. The "Changed" entry now lists three effects:
The PR description has the same list. 3. Scope. Both entries now say "when SQLite is linked into the extension (the precompiled gems and the default build)". The fix entry adds that 4. "As documented".
5. C comment. Fixed:
6. Testing hook.
House style. Noted in the description: add the upstream PR number to both CHANGELOG entries once it exists. Verification at 15e4a37:
Housekeeping: GitHub resolves a plain |
MahtraDR
left a comment
There was a problem hiding this comment.
PR Review: fix: point SQLite's Sleep at kernel32 on Windows
Reviewer: Claude · 15e4a37 · 2026-11-06
Thanks. I re-checked all six items at 15e4a37, and each one is fixed in a way that answers the point.
- The CHANGELOG and the
busy_handler_timeout=rdoc now both say it's only safe when no other thread uses the waiting connection. The rdoc text shows up in the generated docs. - The three upgrade consequences are in the "Changed" entry and the PR description. The figures match what I measured in round 1.
- Both entries now say the change applies when SQLite is linked into the extension.
- The
busy_timeoutrdoc now states the GVL hold, the deferred interrupts and the starved same-process lock holder, so the test header's "as documented" is true.- Your side note is right, and it predates this PR. With the repo's rdoc,
busy_timeout=renders with no description at all, onmaintoo, so the new text is in the source but not in the rendered docs. Switching thecall-seqto the multi-line form didn't fix it. That's worth raising upstream separately.
- Your side note is right, and it predates this PR. With the repo's rdoc,
- The C comment now gives the real reason for the runtime lookup, and the list of sleep sites includes winClose and the WAL retry.
- The return codes are checked, and the warning works.
- I compiled the new function body verbatim against stubs. All eight cases behave as expected: no warning when any VFS accepts the override, and exactly one warning when none does, when kernel32 or
Sleepcan't be found, or when the VFS is too old. The check fails if I remove the== SQLITE_OKtest. - The only case where it would warn without cause is a new header used with a pre-3.7.6 runtime DLL, which isn't a realistic setup.
- The SQLCipher Windows job, which runs this code against a separate DLL, printed no warning.
- I compiled the new function body verbatim against stubs. All eight cases behave as expected: no warning when any VFS accepts the override, and exactly one warning when none does, when kernel32 or
One correction to my own round-1 wording, which you followed: I listed "mswin" among the builds that use SQLite as a separate DLL. That's true for mswin with --enable-system-libraries, which is the only mswin setup CI tests. extconf doesn't force it, though, so a vendored mswin build (if that works at all) would be affected like the default build. The fix runs there too, so nothing needs to change.
CI at 15e4a37 has 131 passing jobs and 3 failures, none caused by this PR:
sqlcipher (windows, 3.3)failedtest_replacing_a_collation_releases_the_previous_comparator, an ObjectSpace count afterGC.start. It fails the same way on upstreammainat this PR's base commit (run 41797589777) and on the v2.10.2 tag.sqlcipher (windows, mingw)is the same MSYS2 package failure as before.arm64-darwin 3.3timed out downloading the gem artifact before any test ran. Nothing in this commit compiles differently on macOS, and the job passed at 5bd933d. A re-run should turn it green.
The issue-only CHANGELOG citation is noted in your description for when it goes upstream.
|
CI at 15e4a37: 131 jobs pass, 3 fail, none from this change.
All Windows jobs with SQLite linked into the extension pass, including the native-gem tests. On the Ruby 4.0 job ( |
Only --enable-system-libraries builds (which SQLCipher builds imply) use SQLite as a separate DLL and already had kernel32's Sleep. CI's mswin job uses that option, but a vendored mswin build would be affected like the default build; the fix runs there too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Opened upstream as sparklemotion#771, from this same branch. The rdoc rendering problem is filed separately as sparklemotion#772. |
Fixes
#766.Problem
On Windows, SQLite's own sleeps release the GVL and can deliver interrupts while SQLite holds a connection's mutex. That contradicts
Database#busy_timeout's documentation ("holds the GVL") and can deadlock the whole process:Thread#raise,Thread#killor Timeout lands in the sleep and unwinds through SQLite's C frames, leaving the mutex locked for the next caller.We hit the first in production: whole-process freezes at 0% CPU in a multi-process app. A full-memory dump showed the GVL owned by a thread in
EnterCriticalSectionon a connection mutex, and that mutex owned by a thread inrb_w32_Sleep, inside SQLite'swinLockretry, waiting for the GVL. The write-up is in#766and inelanthia-online/lich-5#2254(https://github.com/elanthia-online/lich-5/issues/2254#issuecomment-6647684262).Cause
"Sleep"entry of its system-call table (osSleep) while holdingdb->mutex:winLock's PENDING-lock retry (osSleep(1)between 3 attempts), which happens whenever another process holds the lock, even withbusy_timeout(0);winRetryIoerr;busy_timeout(N)(sqliteDefaultBusyCallback→winSleep);winClose's retry.mkmflinks Ruby's import library before kernel32. The Ruby DLL exports aSleepsymbol that forwards torb_w32_Sleep:win32/mkexports.rbturns#define Sleep(msec) rb_w32_Sleep(msec)ininclude/ruby/win32.hinto an export. So that"Sleep"slot resolves torb_w32_Sleep.Sleepfrom the Ruby DLL: 13 builds from 1.3.13 to 2.10.2.--enable-system-libraries) gets kernel32'sSleep.rb_w32_Sleep(thread_win32.c) runs the sleep inBLOCKING_REGION, which releases the GVL and, throughunblock_function_set, runsRUBY_VM_CHECK_INTS.Fix
In
Init_sqlite3_native, on Windows, aftersqlite3_initialize(), look up kernel32'sSleepwithGetProcAddressand install it in SQLite's system-call table throughxSetSystemCall.GetProcAddress: inside the extension, writingSleepwould give you theruby/win32.hmacro and the same Ruby import, so the address has to be looked up at run time.win32,win32-longpath,win32-none,win32-longpath-none) share one table. Each is tried in case some are compiled out.Sleepalready is kernel32's.#if defined(_WIN32) && SQLITE_VERSION_NUMBER >= 3007006, sincexSetSystemCallneeds VFSiVersion3. Nothing changes on other platforms.Behaviour change
This applies only to builds with SQLite linked into the extension: the precompiled gems and the default build. Builds with
--enable-system-libraries(which SQLCipher builds imply) already used kernel32'sSleep. That includes CI's mswin job, but a vendored mswin build would be affected like the default build, and the fix runs there too. On those Windows builds, SQLite's waits now hold the GVL, as documented and as on Linux and macOS. Upgraders will notice:busy_timeoutwait. If another connection in the same process holds the lock (a connection pool, say), its thread can't run to release it, so the wait ends inBusyExceptionafter the full timeout.Timeout.timeout,Thread#raise,Thread#killand Ctrl-C take effect only once the wait ends. ATimeoutshorter than the busy timeout ends inBusyExceptionrather thanTimeout::Error.winRetryIoerr), and up to 200 ms when closing (winClose).busy_handler_timeout=doesn't help shared connections. It lets other threads run during the wait only if they don't use the waiting connection. SQLite runs the Ruby handler while holding the connection's mutex, so a thread that calls into the connection meanwhile hangs the process, on every platform. If threads share a connection, keepbusy_timeout, or give each thread its own connection.These are in the CHANGELOG, in
Database#busy_timeout's rdoc (GVL and interrupts), and inDatabase#busy_handler_timeout='s rdoc (the shared-connection caveat).Alternatives considered
Sleepand install it withxSetSystemCall. It doesn't depend on link order or toolchain.xSetSystemCallis documented as a testing aid, and the set of overridable calls may change between SQLite releases. Mitigations: the return codes are checked, and the extension warns at load time if no VFS accepted the override.test/test_sqlite_sleep.rbalso fails on the Windows CI jobs if the override stops working, which the tests-only run demonstrated.-lkernel32right after the vendoredlibsqlite3.ainextconf.rb. A MinGW cross-link test showed this binds kernel32'sSleep.--enable-system-librariesdoes). ItsSleepcomes from kernel32.Option 1 can also be combined with 2 later, if the maintainers prefer to fix the link and keep the override as a guard.
Tests (
test/test_sqlite_sleep.rb)busy_timeoutwait doesn't let another Ruby thread run (a ticker thread counts at most a tick or two over a 300 ms wait).Thread#raiseaimed at a thread in abusy_timeoutwait leaves the connection usable from another thread.Tests 2 and 3 run in a child process with a 30 s timeout, because a deadlock stops every thread of the process. They use
spawnrather thanfork(unlikeassert_connection_usable_from_another_thread), so they run on Windows too.Checking that the tests catch the bug. The bug can't be reproduced natively on Linux, so I used a small test-only
LD_PRELOADshim. It routesnanosleepcalls made fromsqlite3_native.sothroughrb_thread_call_without_gvl, which releases the GVL and checks interrupts, asrb_w32_Sleepdoes. With it, all three tests fail as they would on Windows:Without the shim, all three pass.
Test 1 is skipped on non-CRuby. TruffleRuby has no GVL, so other threads legitimately run during the wait. The two subprocess tests run everywhere.
Results:
test (windows, 3.2–4.0, disable)(vendored SQLite)test source (windows, 3.2–4.0, disable)x64-mingw-ucrt 3.2–4.0(native gem tests)test (windows, *, enable),mswin(both--enable-system-libraries: SQLite as a separate DLL)On the Ruby 4.0 Windows job without the fix, the failures were:
Skip counts are the same in both runs, so the tests ran rather than being skipped. Jobs that link SQLite as a separate DLL pass either way, because that DLL's
Sleepcomes from kernel32.This PR's head (
5bd933d, CI on this PR): 133 jobs pass, 1 fails. The failure issqlcipher (windows, mingw), whose MSYS2 package install also fails onmain. All Windows, macOS, Linux, TruffleRuby and native-gem jobs pass.One earlier run (before the TruffleRuby skip) also had a single failure of the existing
test_busy_timeoutwall-clock check (0.996 >= 1.0) onx86_64-linux-gnu 4.0. It passed on this head.Notes
getpid(rb_w32_getpid), which the extension imports too. It's harmless and left alone.Sleepto the Ruby export as well. That's outside SQLite and not touched here.#766, #NNN), so add the upstream PR number once it exists.