Repository navigation
Fix six high-severity defects in the loader, pybridge, jit options and examples - #80
Closed
nelson2005 wants to merge 10 commits into
Closed
nelson2005 wants to merge 10 commits into
nelson2005 wants to merge 10 commits into
Conversation
extract_connection_ptr ran SELECT 1 through the extracted pointer as a liveness check. On the caller's own connection that query closed whatever result the connection was still streaming, so every later fetch came back short without an error (5000 rows pending, 0 fetched), and it rewrote the connection's profiling_output. It held the GIL while waiting on the connection's lock, so it could deadlock against another thread's query on that connection, and inside an aborted transaction it reported a valid pointer as failed validation. The check could not catch a stale pointer either. The exact-type, coordination and null guards stay.
…scope The fallback loader opened each standalone candidate RTLD_GLOBAL and only then checked its version. A refused library was never unloaded, and numbox resolves every JIT call by name through that global scope, first definition wins. After a refusal, setting NUMBDUCK_LIBDUCKDB and importing again in the same process, as the refusal text advises, recorded the accepted library while every JIT call still ran the refused build, and libraries_coordinated() reported True. A candidate is now opened RTLD_LOCAL to check that it exports the C API and that its version matches the wheel's, and loaded RTLD_GLOBAL only after it passes. The download path goes through the same check.
Every binding and allocator was compiled with NUMBDUCK_JIT_OPTIONS, whose default was {"cache": true} with no nogil, so ducklib.duckdb_query called from Python kept the GIL for the whole query while the calling thread waited for DuckDB's worker threads. A worker that needs the GIL, such as a Python UDF on the same connection or a numba @cfunc reporting an exception it swallowed, waited for it forever: SELECT sum(plus_one(x)) over 250000 rows with two threads never returned. The default is now {"cache": true, "nogil": true}, and NUMBDUCK_JIT_OPTIONS overrides individual defaults instead of replacing them all, so {"cache": false} keeps nogil. numba's cache key holds no compile options, so a cache warmed before this change keeps serving bindings compiled without nogil until ducklib.py changes or the cache is cleared.
numba's cache index key holds no compile options, so machine code compiled under one NUMBDUCK_JIT_OPTIONS was served as-is to a process running under another. One run under an admitted option poisoned the cache for every later default process: after a writer with no_cfunc_wrapper, every default import aborted with LLVM ERROR: Symbol not found: numbox_pxy_duckdb_array_type_array_size_... . Any option that changes the default compile options now turns the cache off, so such a process neither writes nor reads the shared cache, with a RuntimeWarning when the cache was asked for explicitly. Re-run against this tree, the same writer leaves no cache file and both default readers import.
numba frees an array right after its last use, and the address array_data_p returns keeps nothing alive. In ducklib.duckdb_close(array_data_p(db)) the buffer's last use is array_data_p, so numba freed it before duckdb_close read the handle out of it and wrote NULL back. The four JIT lifecycle tests did this for every close, disconnect and destroy, and online_scoring's three raise branches did it for the result and the chunk, whose buffers are dead on those branches. It passed only because a freed block's bytes survive until reuse: under an allocator that overwrites freed memory all six tests crash. The JIT tests now read every slot back after destroying through it, which also checks that the destroy nulled it, and online_scoring destroys through _release, which takes the buffers themselves. A new test reruns the six under a preloaded poison-on-free allocator on Linux, and examples/README.md states the rule.
The C API reads every char* as NUL-terminated UTF-8, and numbox's get_unicode_data_p hands over CPython's internal storage instead: Latin-1, UCS-2 or UCS-4 depending on the widest character, which is UTF-8 only for ASCII. The examples passed every function name and SQL string that way, so text copied from them into anything that is not ASCII reached DuckDB cut short or as invalid UTF-8: a path with a euro sign opened the wrong file, and a table named café got the Latin-1 bytes. The examples now use numbox's c_string, which encodes the text, examples/README.md says why and what to do inside @njit, and a test sends a non-ASCII path, table name and bound value through it and reads them back. The same test body fails at its first step when the old helper stands in for c_string.
…TF-8 A native Python UDF costs tens of microseconds a row, so the 250000 calls took 20 s here and passed the 120 s timeout on a Windows runner. A filter keeps the scan's three row groups, so DuckDB's worker threads still reach the UDF and need the GIL, and cuts the calls to about 4000: 1.2 s with the fix, and the old behaviour still deadlocks in 3 of 3 runs. DuckDB drew its progress bar on the slow query, and on Windows the subprocess output was decoded as cp1252, which cannot read the bar's block characters: the reader thread died and stdout came back None. The script turns the progress bar off and the output is decoded as UTF-8.
With one thread DuckDB runs the whole query, the Python UDF included, on the calling thread, which already holds the GIL, so the bindings compiled without nogil completed 3 of 3 runs at threads=1 and deadlocked 9 of 9 at 2, 4 and 8; the shipped defaults completed 12 of 12. The script pins two threads so that a second one is in play whatever the host's core count, and the docstring now says so.
On macOS the allocator interposes free through dyld's __interpose section and sizes blocks with malloc_size, built with cc -dynamiclib and loaded with DYLD_INSERT_LIBRARIES. dyld drops that variable for a restricted python, which would leave the six tests running unpoisoned and passing for nothing, so the library announces itself as it loads and the test skips when the announcement is missing from the subprocess's stderr. On Linux the announcement is the only change, and the pre-fix tree still dies with SIGBUS under the rebuilt library.
…irst An interposed free, loaded through DYLD_INSERT_LIBRARIES, reached numba's frees under the python.org and conda-forge interpreters on a macos-latest runner but not under Homebrew's, where the six tests passed with nothing poisoned: libmalloc zeroes freed blocks by default for an executable built against a recent SDK, and a zeroed buffer hands the destroy call a NULL handle, which it checks and skips. On macOS the test now sets MallocScribble, with MallocZeroOnFree off, and libmalloc overwrites every block it frees. Rather than trust either allocator, the test first fills and drops a numba array under the same environment and reads the freed bytes back: when the pattern survives, as it does for a restricted python that ignores the variables, or was zeroed, the run is skipped rather than passed. The C allocator is Linux-only again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six defects, one commit each, each with a test that fails without it.
extract_connection_ptrran a query on the caller's connection (695c83b). ItsSELECT 1liveness check closed whatever result the connection was still streaming, so later fetches came back short with no error (5000 rows pending, 0 fetched). It also rewrote the connection'sprofiling_output, could deadlock against another thread's query on the same connection while holding the GIL, and inside an aborted transaction reported a valid pointer as failed validation. It could not catch a stale pointer either. The check is gone; the exact-type, coordination and null guards stay.A refused standalone libduckdb kept serving the JIT (
daf1568). The fallback loader opened each candidateRTLD_GLOBALbefore checking its version, and numbox resolves JIT calls by name through that scope, first definition wins. After a refusal, settingNUMBDUCK_LIBDUCKDBand importing again in the same process recorded the good library while every JIT call still ran the refused one. A candidate is now openedRTLD_LOCALfor the checks and loaded globally only once it passes.Bindings called from Python held the GIL for the whole query (
98cedec). A DuckDB worker thread that needs the GIL, such as a Python UDF on the same connection, waited for it forever:SELECT sum(plus_one(x))over 250000 rows with two threads never returned. The default jit options are now{"cache": true, "nogil": true}, andNUMBDUCK_JIT_OPTIONSoverrides individual defaults instead of replacing them all.One run under non-default jit options poisoned the disk cache (
a0597ba). numba's cache key holds no compile options, so after a run withno_cfunc_wrapperevery default import aborted withLLVM ERROR: Symbol not found. Only the default compile options use the cache now, with aRuntimeWarningwhen the cache is asked for together with others.Destroy calls read out-parameter buffers numba had already freed (
d35add9). Inducklib.duckdb_close(array_data_p(db))the buffer's last use isarray_data_p, so numba frees it beforeduckdb_closereads the handle. The JIT lifecycle tests and the raise branches inonline_scoring.pydid this. They passed only because freed bytes survive until reuse, and all six crash under an allocator that overwrites freed memory. The tests now read each slot back after the destroy, which also checks that it was nulled,online_scoring.pydestroys through a helper that takes the buffers, and a new test reruns the six under a poison-on-free allocator on Linux and macOS.The examples passed text as CPython's internal storage (
5d46620).get_unicode_data_pis UTF-8 only for ASCII, so a path, table name or value copied from the examples with any other character reached DuckDB cut short or as invalid UTF-8. The examples use numbox'sc_stringnow,examples/README.mdexplains this and the buffer rule, and a test round-trips a non-ASCII path, table name and bound value.A numba cache warmed before the
nogildefault keeps serving the old bindings untilducklib.pychanges or the cache is cleared, since the cache key does not see compile options.The suite passes locally on duckdb 1.2.2, 1.3.2, 1.4.0, 1.5.4 and 1.5.5, numba 0.60.0, 0.65.1 and 0.67.0, numbox 0.6.2 and 0.7.5, and Python 3.10, 3.12 and 3.14: 222 passed and 2 skipped, 220 and 4 on duckdb 1.2.2.