diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e96859..9612575 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,6 +38,64 @@ jobs: env: PYO3_NO_PYTHON: 1 + # What the public surface was, against what it is. Griffe reads the + # package the way a reader does, out of the .py files and the .pyi + # that declares the compiled half, and it names what moved, changed + # shape or went away. Nothing is built here on purpose: the declared + # API is what the stub says, so the stub is what gets compared, and a + # stub that has drifted from the extension is what the typing tests in + # the suite are for. + # + # A break is allowed. It has to be paid for with a version bump, which + # is the rule cargo semver-checks applies to the engine and the rule + # api-extractor's committed report applies to the TypeScript client: + # the change is fine, the change happening quietly is not. So this + # compares the version too and steps aside once it has moved, after + # printing what moved with it. + api: + runs-on: ubuntu-latest + steps: + # Griffe builds a worktree at the baseline, so it wants the ref + # and not just the one commit under test. + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - run: pip install griffe==2.2.0 + # The gate is validated the only way a gate can be: a break has to + # fail it. Renaming a class the DB-API layer inherits from is the + # cheapest break that is unambiguously one, and the tree is put + # back before anything else looks at it. + - name: A break the gate is meant to catch, caught + env: + BASE_REF: ${{ github.base_ref }} + run: | + base="${BASE_REF:+origin/$BASE_REF}" + base="${base:-HEAD^}" + sed -i 's/^class DataError(/class DataErrorRenamed(/' python/zudb/errors.py + set +e + griffe check zudb -s python --against "$base" + rc=$? + set -e + git checkout -- python/zudb/errors.py + test $rc -ne 0 || { echo "the api gate did not fire on a removed name"; exit 1; } + - name: The surface, against what it was + env: + BASE_REF: ${{ github.base_ref }} + run: | + base="${BASE_REF:+origin/$BASE_REF}" + base="${base:-HEAD^}" + was=$(git show "$base:pyproject.toml" | sed -n 's/^version = "\(.*\)"/\1/p' | head -1) + now=$(sed -n 's/^version = "\(.*\)"/\1/p' pyproject.toml | head -1) + if [ "$was" != "$now" ]; then + echo "version moved from $was to $now, so a break here is one that was declared" + griffe check zudb -s python --against "$base" || true + exit 0 + fi + griffe check zudb -s python --against "$base" + test: strategy: fail-fast: false @@ -62,6 +120,157 @@ jobs: - run: pip install pytest griffe ipython - run: pytest + # The same suite again, over an extension built with AddressSanitizer. + # + # There is no `unsafe` in this crate, which is the reason to run this + # rather than the reason not to: what a binding gets wrong is not + # arithmetic on a raw pointer but a buffer read after the object that + # owned it was collected, or an engine allocation freed on one side of + # the boundary and touched from the other. Neither is an `unsafe` + # block here and both are a use-after-free. + # + # ASan's runtime has to be loaded before anything it instruments, and + # the extension is opened by `import` long after python has started, + # so it is preloaded rather than linked: `-Zexternal-clangrt` tells + # rustc not to bundle its own copy and LD_PRELOAD supplies clang's. + # `--target` is what keeps the flags off the build scripts, which are + # host programs with no runtime preloaded and would not link. + # Leak detection is off, because LSan reports the interpreter's own + # hundred one-time allocations and cannot be told to stop; the job + # below is where leaks are counted, with a tool that can. + sanitizer: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - uses: Swatinem/rust-cache@v2 + # The instrumented build is nightly, because `-Zsanitizer` is, and + # it is the only thing here that is: what ships is built by the + # pinned compiler in every other job. + - run: rustup toolchain install nightly --profile minimal + - run: sudo apt-get update && sudo apt-get install -y libclang-rt-18-dev + # pandas and pyarrow by name rather than through the extra that + # names them, because installing the extra would build this + # extension from source to get at its metadata and this job builds + # its own a step later. They are here at all because two of the + # whole programs the README publishes call to_pandas, and a job + # that skipped them would be watching a smaller suite than the + # one it claims to watch. + - run: pip install maturin pytest ipython pandas pyarrow + - name: The extension, instrumented + env: + RUSTUP_TOOLCHAIN: nightly + RUSTFLAGS: -Zsanitizer=address -Zexternal-clangrt + CC: clang + CXX: clang++ + run: | + maturin build --target x86_64-unknown-linux-gnu --out dist + pip install --force-reinstall --no-deps dist/*.whl + - name: The suite, watched + run: | + runtime=$(clang -print-file-name=libclang_rt.asan-$(uname -m).so) + test -f "$runtime" + LD_PRELOAD="$runtime" ASAN_OPTIONS=detect_leaks=0:abort_on_error=1 \ + pytest -m "not timing" + + # The third tool over the same suite, and the one that watches what + # the sanitizer cannot. ASan instruments the source it compiles, so it + # sees nothing the interpreter does with the memory it hands the + # extension; Valgrind instruments the instructions that run, so both + # sides of the boundary are watched and so is every prebuilt thing + # either of them links. It also reports a read of memory nobody wrote, + # which ASan does not look for at all. + # + # PYTHONMALLOC=malloc is what makes this readable. CPython pools small + # objects behind its own allocator by default, so every allocation + # this extension makes through the interpreter would arrive as one + # 256KB arena and nothing inside it could be attributed to anybody. + # + # tools/valgrind.supp is one rule and says what it leaves out. The + # gate is validated the only way a gate can be: a deliberate leak + # through ctypes fails it. + # + # Definite leaks and not possible ones, which is the same pair of + # flags the TypeScript client's job carries and for a sharper reason + # here. Over the whole suite this counts 420 possible losses and zero + # definite ones, and 417 of the 420 are pyarrow registering compute + # kernels into a static table it never tears down. A possible loss is + # a block whose only surviving pointer is into its middle, which is + # what a registry of C++ objects looks like from the outside and what + # almost nothing this extension allocates looks like. Counting them + # would mean either a red job or a suppression naming pyarrow, and + # naming a dependency in a suppression file is how a suppression file + # starts growing. What it costs is a Rust leak that happens to leave + # an interior pointer behind, and the suite counts live connections + # and process descriptors from the other side for that. + leaks: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-python@v6 + with: + python-version: "3.14" + - uses: Swatinem/rust-cache@v2 + - run: sudo apt-get update && sudo apt-get install -y valgrind + # The release build, with its symbols left on. Valgrind wants the + # instructions the extension actually ships, so this is the same + # optimised build and not an instrumented one, which would be a + # different program with a different allocator underneath it. What + # changes is only that the symbol table survives: a leak report + # whose every frame reads ??? says a number and nothing a person + # can act on, and the suppression file cannot name a function it + # has no name for. + # + # Three settings and not one, because two things strip this wheel + # and only one of them is cargo. `strip = true` in pyproject.toml + # is maturin's own, applied after the build and to the artifact + # rather than through the profile, so a cargo profile that says to + # keep the symbols is a cargo profile maturin then throws away. + # That is what happened the first time this job ran the suite: one + # eighty byte record out of pyo3's type constructor, named in the + # suppression file, reported with ??? on every frame and matched + # by nothing. + # + # Every optional dependency except polars, which starts a thread + # pool the moment it is imported and holds a block of thread-local + # state per worker for the life of the process. Valgrind reports + # all of it and none of it belongs to this client, and the one + # test that wants polars skips itself when it is missing. Pandas + # and pyarrow stay because the README examples this suite runs go + # through them. + - run: pip install ".[pandas]" pytest ipython + env: + MATURIN_STRIP: "false" + CARGO_PROFILE_RELEASE_STRIP: "false" + CARGO_PROFILE_RELEASE_DEBUG: "1" + # Said here rather than trusted, because the whole job rests on it + # and the way it fails is an hour of valgrind followed by a report + # nobody can read. The section is what valgrind reads a frame's + # name out of, and a stripped object has neither it nor a symbol + # table to fall back on. + - name: The symbols the report is read with + run: | + set -eu + so=$(python -c 'import zudb, pathlib; print(pathlib.Path(zudb.__file__).parent / "_zudb.abi3.so")') + readelf -S "$so" | grep -q debug_info \ + || { echo "$so carries no debug info, so every frame of the report would read ???"; exit 1; } + - name: A leak the job is meant to catch, caught + run: | + set +e + PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ + --show-leak-kinds=definite --errors-for-leak-kinds=definite \ + --suppressions=tools/valgrind.supp -q \ + python -c 'import ctypes; ctypes.CDLL("libc.so.6").malloc(4096)' + test $? -eq 1 || { echo "the leak gate did not fire on a leak"; exit 1; } + - name: The suite, counted + run: | + PYTHONMALLOC=malloc valgrind --error-exitcode=1 --leak-check=full \ + --show-leak-kinds=definite --errors-for-leak-kinds=definite \ + --suppressions=tools/valgrind.supp -q \ + python -m pytest -m "not timing" + # The shared corpus, which is the same 945 cases the engine runs # against itself and the eight other clients run against theirs. It is # a job of its own because it needs a second checkout, and it runs on diff --git a/pyproject.toml b/pyproject.toml index b561cc7..84c3deb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -74,6 +74,14 @@ addopts = "-q" # holds `conformance`, which is a development tool rather than something # the wheel ships. pythonpath = ["tools", "."] +# Tests that assert on a wall clock: that two connections overlap, that +# an interrupt is felt inside a budget, that appending beats inserting +# by a margin. They are the tests that say this client is fast, and +# they are meaningless under a sanitizer, which makes every instruction +# slower by a factor nobody controls and the two sides of a ratio +# slower by different ones. The jobs that run under one deselect them +# by this marker and the ordinary jobs run them all. +markers = ["timing: asserts on elapsed time, so a sanitizer run has to skip it"] [tool.ruff] target-version = "py311" diff --git a/python/zudb/magic.py b/python/zudb/magic.py index 2d65074..a75cf9d 100644 --- a/python/zudb/magic.py +++ b/python/zudb/magic.py @@ -39,6 +39,7 @@ from __future__ import annotations +import os import shlex from pathlib import Path from typing import Any @@ -51,6 +52,31 @@ __all__ = ["ZuMagics", "load_ipython_extension"] +def words(line: str) -> list[str]: + r"""A magic's line, split the way the shell of this machine splits. + + `shlex.split` is the obvious thing to call and it is wrong on + Windows, where a backslash is a path separator rather than an + escape. `%gql C:\data\social.zu1` comes back out of it as + `C:datasocial.zu1`, and the message a person then reads is that + the system cannot find a file they can see in the directory + listing in front of them. + + So the escape character is the platform's. Quoting is not: a path + with a space in it is quoted the same way everywhere, because that + is what a person typing into a notebook cell will have done. The + comment character is turned off for the same reason `shlex.split` + turns it off, which is that `#` is a legal character in a file name + and a line magic is not a script. + """ + lexer = shlex.shlex(line, posix=True) + lexer.whitespace_split = True + lexer.commenters = "" + if os.name == "nt": + lexer.escape = "" + return list(lexer) + + @magics_class class ZuMagics(Magics): """The two magics, and the connection `%gql` opened if it opened one.""" @@ -71,11 +97,11 @@ def gql(self, line: str) -> Connection | None: The connection comes back so that a cell can keep it, and it is closed at the end of the session like any other. """ - words = shlex.split(line) - read_only = "--read-only" in words - closing = "--close" in words - paths = [word for word in words if not word.startswith("-")] - unknown = [word for word in words if word.startswith("-")] + given = words(line) + read_only = "--read-only" in given + closing = "--close" in given + paths = [word for word in given if not word.startswith("-")] + unknown = [word for word in given if word.startswith("-")] for word in unknown: if word not in ("--read-only", "--close"): raise UsageError(f"%gql does not take {word}") @@ -120,16 +146,16 @@ def gql_cell(self, line: str, cell: str) -> Any: def options(self, line: str) -> dict[str, str]: """`--conn`, `--params` and `--out`, each naming a variable.""" - words = shlex.split(line) + given = words(line) taken: dict[str, str] = {} - while words: - word = words.pop(0) + while given: + word = given.pop(0) name = word.removeprefix("--") if name == word or name not in ("conn", "params", "out"): raise UsageError(f"%%gql takes --conn, --params and --out, and not {word}") - if not words: + if not given: raise UsageError(f"--{name} names a variable, and this named none") - taken[name] = words.pop(0) + taken[name] = given.pop(0) return taken def namespace(self) -> dict[str, Any]: diff --git a/src/appender.rs b/src/appender.rs index 3b9c41f..cbdfd70 100644 --- a/src/appender.rs +++ b/src/appender.rs @@ -396,11 +396,29 @@ impl Appender { /// The buffers, for a call that adds to them, which a closed /// appender has no business doing. + /// + /// The connection is checked as well as the appender, because a row + /// appended through a closed connection has nowhere to go and the + /// buffer is the only thing that would take it. Left to the flush, + /// the same call would be refused or not depending on whether the + /// batch happened to fill, which is a rule nobody can hold in their + /// head. It is refused here instead, at the call that made the + /// mistake, whatever the buffer is holding. fn writable(&self, py: Python<'_>) -> PyResult> { let state = self.locked(py)?; if !state.open { return Err(Snag::Finished.raise(py)); } + if self + .conn + .bind(py) + .borrow() + .inner + .lock() + .is_ok_and(|c| c.is_none()) + { + return Err(Snag::Closed.raise(py)); + } Ok(state) } } diff --git a/src/value.rs b/src/value.rs index fbaaedf..9d5f6f8 100644 --- a/src/value.rs +++ b/src/value.rs @@ -478,6 +478,27 @@ fn zone_of(py: Python<'_>, offset: i16) -> PyResult> { /// number against its own spelling would answer nothing and say /// nothing. pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { + nested(value, 0) +} + +/// How deep a parameter may nest before this stops reading it. +/// +/// A list of lists of records is a value somebody meant to send, and a +/// value that contains itself is a call that would otherwise walk until +/// the stack ran out and take the interpreter with it, which it does +/// here rather than in Python and so arrives as a segfault instead of a +/// `RecursionError`. There is no depth between the two that anybody +/// writes on purpose, so the limit is set where a real value never +/// reaches and a cycle always does. +const DEEP: usize = 64; + +fn nested(value: &Bound<'_, PyAny>, depth: usize) -> PyResult { + if depth > DEEP { + return Err(pyo3::exceptions::PyValueError::new_err(format!( + "a parameter nests deeper than {DEEP}, which is what a value that contains itself \ + looks like" + ))); + } if value.is_none() { return Ok(Value::Null); } @@ -531,7 +552,7 @@ pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { return Ok(Value::List( items .iter() - .map(|item| from_py(&item)) + .map(|item| nested(&item, depth + 1)) .collect::>()?, )); } @@ -539,14 +560,14 @@ pub fn from_py(value: &Bound<'_, PyAny>) -> PyResult { return Ok(Value::List( items .iter() - .map(|item| from_py(&item)) + .map(|item| nested(&item, depth + 1)) .collect::>()?, )); } if let Ok(fields) = value.cast::() { let mut out = Vec::with_capacity(fields.len()); for (name, item) in fields.iter() { - out.push((name.extract::()?, from_py(&item)?)); + out.push((name.extract::()?, nested(&item, depth + 1)?)); } return Ok(Value::record(out)); } diff --git a/tests/test_aio.py b/tests/test_aio.py index aab5395..5b59693 100644 --- a/tests/test_aio.py +++ b/tests/test_aio.py @@ -106,6 +106,7 @@ async def test_what_comes_back_is_read_without_awaiting(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_the_loop_runs_while_a_statement_does(tmp_path: Path) -> None: """The claim the module is for. @@ -125,6 +126,7 @@ async def test_the_loop_runs_while_a_statement_does(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_two_connections_run_at_the_same_time(tmp_path: Path) -> None: """A thread each, and the engine puts the GIL down for the work, so two statements together cost about what one costs rather than two. @@ -174,6 +176,7 @@ async def test_cancelling_the_task_stops_the_statement(tmp_path: Path) -> None: @run +@pytest.mark.timing async def test_a_cancelled_statement_leaves_the_connection_free(tmp_path: Path) -> None: """The reason the cancellation waits for the statement it stopped. diff --git a/tests/test_appender.py b/tests/test_appender.py index 6f3ed24..87b5176 100644 --- a/tests/test_appender.py +++ b/tests/test_appender.py @@ -432,6 +432,7 @@ def run() -> None: COMPARED = 200 +@pytest.mark.timing def test_appending_beats_inserting_by_the_margin_that_makes_it_worth_having( tmp_path: Path, ) -> None: diff --git a/tests/test_arrow.py b/tests/test_arrow.py index 48510ad..ac8b496 100644 --- a/tests/test_arrow.py +++ b/tests/test_arrow.py @@ -243,6 +243,7 @@ def test_polars_reads_the_same_rows(loaded: zudb.Connection) -> None: assert frame["name"].to_list() == ["ada", "grace", "kay"] +@pytest.mark.timing def test_python_keeps_running_while_a_result_becomes_arrow(tmp_path: Path) -> None: rows = 400_000 zudb.load( diff --git a/tests/test_interrupt.py b/tests/test_interrupt.py index 5c4cce8..a8e0c19 100644 --- a/tests/test_interrupt.py +++ b/tests/test_interrupt.py @@ -14,8 +14,10 @@ from __future__ import annotations +import _thread import os import signal +import sys import threading import time @@ -35,6 +37,40 @@ SETTLED = 0.2 +def press() -> None: + """`Ctrl-C`, from inside the process that is going to feel it. + + Everywhere but Windows that is the signal a terminal sends, sent the + way a terminal sends it: to this process, for the main thread to + answer. + + Windows has no way to send one process a SIGINT. `os.kill` there is + `TerminateProcess` for every signal except the two console events, + so `os.kill(os.getpid(), SIGINT)` does not deliver a press, it kills + the process, which is what it had been doing to this suite on every + Windows run: five lines of dots and an exit code, no failure and no + summary, because there was no interpreter left to write one. The two + console events are no better, since a console event goes to every + process attached to the console and on a build machine that is the + build. + + So Windows presses the key the way CPython's own console handler + does when a real press arrives: `PyErr_SetInterrupt`, which + `_thread.interrupt_main` is the spelling of. What that leaves + uncovered is one hop, from the operating system into the C runtime, + and nothing running inside this process can cover that hop without + taking the process with it. Everything after the hop is the same on + both platforms and is all of the code this repository wrote: a + tripped SIGINT, a statement deep in the executor, and the next + `PyErr_CheckSignals` turning one into a `KeyboardInterrupt` on the + main thread inside the budget. + """ + if sys.platform == "win32": + _thread.interrupt_main() + else: + os.kill(os.getpid(), signal.SIGINT) + + def after(delay: float, do) -> threading.Thread: """Runs `do` on a thread of its own, `delay` from now, and answers the thread so a test can join it.""" @@ -48,16 +84,15 @@ def wait() -> None: return thread +@pytest.mark.timing def test_a_press_raises_keyboard_interrupt_within_the_budget(throng: zudb.Connection) -> None: pressed: list[float] = [] - def press() -> None: + def timed_press() -> None: pressed.append(time.perf_counter()) - # The signal a terminal sends, sent the way a terminal sends it: - # to the process, for the main thread to answer. - os.kill(os.getpid(), signal.SIGINT) + press() - presser = after(SETTLED, press) + presser = after(SETTLED, timed_press) with pytest.raises(KeyboardInterrupt): throng.execute(WORK) felt = time.perf_counter() @@ -66,7 +101,7 @@ def press() -> None: def test_the_connection_is_the_same_afterwards(throng: zudb.Connection) -> None: - presser = after(SETTLED, lambda: os.kill(os.getpid(), signal.SIGINT)) + presser = after(SETTLED, press) with pytest.raises(KeyboardInterrupt): throng.execute(WORK) presser.join(timeout=30) @@ -76,6 +111,7 @@ def test_the_connection_is_the_same_afterwards(throng: zudb.Connection) -> None: assert throng.execute("MATCH (p:person) RETURN count(p) AS n").fetchone() == (12_000,) +@pytest.mark.timing def test_interrupt_stops_a_statement_from_another_thread(throng: zudb.Connection) -> None: asked: list[float] = [] @@ -123,7 +159,7 @@ def test_an_ask_with_nothing_running_does_not_end_the_next_statement( def test_a_press_with_nothing_running_is_pythons_to_deliver(social: zudb.Connection) -> None: with pytest.raises(KeyboardInterrupt): - os.kill(os.getpid(), signal.SIGINT) + press() # Python raises the press at the next thing this thread does, # which is this call, and the statement it stopped is none. for _ in range(1000): @@ -137,6 +173,7 @@ def test_interrupt_on_a_closed_connection_says_so(social: zudb.Connection) -> No social.interrupt() +@pytest.mark.timing def test_a_connection_answers_while_it_is_busy(throng: zudb.Connection) -> None: """Asking a connection how it is going does not queue behind the statement it is going through.""" @@ -165,6 +202,7 @@ def test_rows_read_holds_what_the_last_statement_cost(social: zudb.Connection) - assert social.rows_read >= 3 +@pytest.mark.timing def test_a_statement_that_finishes_is_not_slowed_by_being_watched(social: zudb.Connection) -> None: """The thread a statement runs on is kept rather than made, so a small statement on the main thread costs about what it costs on any @@ -198,7 +236,7 @@ def test_a_press_during_a_statement_that_finishes_first_is_still_raised( """A press is never swallowed. The statement was over before it arrived, so Python raises it at the next thing this thread does.""" with pytest.raises(KeyboardInterrupt): - os.kill(os.getpid(), signal.SIGINT) + press() social.execute("MATCH (p:person) RETURN p.name AS n") for _ in range(1000): pass diff --git a/tests/test_magic.py b/tests/test_magic.py index 671d2d7..80a24c6 100644 --- a/tests/test_magic.py +++ b/tests/test_magic.py @@ -14,6 +14,7 @@ from __future__ import annotations +import os from pathlib import Path from typing import Any @@ -24,6 +25,7 @@ from IPython.core.error import UsageError # noqa: E402 from IPython.core.interactiveshell import InteractiveShell # noqa: E402 +from zudb import magic # noqa: E402 @pytest.fixture @@ -196,3 +198,40 @@ def test_a_statement_that_is_wrong_raises_what_it_would_have(shell: Any, tmp_pat shell.run_line_magic("gql", str(tmp_path / "wrong.zu1")) with pytest.raises(zudb.SyntaxError): gql(shell, "MATCH (p:person RETURN p") + + +def test_a_path_with_a_space_in_it_is_quoted_and_opens(shell: Any, tmp_path: Path) -> None: + """Which is the reason the line is lexed rather than split on + whitespace, and the only reason.""" + room = tmp_path / "two words" + room.mkdir() + path = room / "spaced.zu1" + conn = shell.run_line_magic("gql", f'"{path}"') + assert conn.closed is False + assert conn.execute("RETURN 1 AS n").fetchone() == (1,) + + +@pytest.mark.skipif(os.name != "nt", reason="a backslash is only a separator on Windows") +def test_a_windows_path_keeps_its_separators() -> None: + r"""The failure this is here for: `shlex.split` treats a backslash + as an escape, so `C:\data\social.zu1` came out of it as + `C:datasocial.zu1` and the magic then said the system could not + find a file the person could see in front of them.""" + assert magic.words(r"--read-only C:\data\social.zu1") == [ + "--read-only", + r"C:\data\social.zu1", + ] + + +@pytest.mark.skipif(os.name == "nt", reason="a backslash is an escape everywhere else") +def test_a_backslash_is_still_an_escape_where_a_shell_says_it_is() -> None: + """Nothing is taken away from the platforms that were right: a + person on a Unix quotes a space with a backslash and expects that + to go on working.""" + assert magic.words(r"/tmp/two\ words/spaced.zu1") == ["/tmp/two words/spaced.zu1"] + + +def test_a_hash_in_a_name_is_a_name_and_not_a_comment() -> None: + """A line magic is not a script, and `#` is a legal character in a + file name on every platform this runs on.""" + assert magic.words("a#b.zu1 --read-only") == ["a#b.zu1", "--read-only"] diff --git a/tests/test_misuse.py b/tests/test_misuse.py index d8ada2e..faaf14a 100644 --- a/tests/test_misuse.py +++ b/tests/test_misuse.py @@ -1,6 +1,6 @@ """Deliberately wrong programs, and what each of them is told. -DX2 asks for a misuse suite in both clients: no crash, no leak, and a +DX3 asks for a misuse suite in every client: no crash, no leak, and a clear error for every program that is wrong on purpose. Clear is the hard word of the three, so it is spelled out here as three things a message has to do. It names the thing the caller named, being the file @@ -19,10 +19,18 @@ caller who mistyped one to file a bug would be the wrong answer twice. No crash is the suite running at all. No leak is checked from outside -the call that would cause one, twice: every case is followed by a read -on the connection it was aimed at, and the failing connects are +the call that would cause one, three ways: every case is followed by a +read on the connection it was aimed at, the failing connects are repeated five hundred times, which is past the descriptor limit a -process starts with. +process starts with, and the descriptors themselves are counted where +the operating system will say. + +The lifecycle tests are the second half DX3 asks for. A misuse suite +watches what a wrong program is told; a lifecycle suite watches what a +right program leaves behind, which is the failure nobody sees until the +loop has run for a week. Opened and closed, opened and dropped, closed +with something still open on it: each of the three is counted rather +than described. The last two tests are the half of a misuse suite that is usually missing. The programs that look wrong and are not, each of which is a @@ -134,6 +142,12 @@ def read_only(where: Path) -> zudb.Connection: TypeError, ("a parameter cannot be a object", "zu holds"), ), + Misuse( + "passes a parameter that contains itself", + lambda conn, tmp: conn.execute("RETURN $x AS x", {"x": knot()}), + ValueError, + ("nests deeper than 64", "contains itself"), + ), Misuse( "passes the parameters as a list", lambda conn, tmp: conn.execute("RETURN $x AS x", [1, 2]), @@ -247,6 +261,14 @@ def read_only(where: Path) -> zudb.Connection: ) +def knot() -> dict[str, object]: + """A dict that holds itself, which is the value a conversion written + the obvious way walks until the stack runs out.""" + tied: dict[str, object] = {} + tied["self"] = tied + return tied + + def closed_appender(conn: zudb.Connection) -> zudb.Appender: """An appender that has been closed, which is the state a `with` block leaves one in.""" @@ -332,6 +354,131 @@ def test_five_hundred_failed_connections_leave_nothing_open(tmp_path: Path) -> N assert alive == [] +def descriptors() -> int | None: + """How many files this process has open, or `None` where the + operating system will not say. + + Linux and macOS both keep the answer in a directory, at different + paths, and Windows keeps it nowhere a process can read. A count is + not a leak detector on its own, which is why what the tests below + compare is the count before a thousand cycles against the count + after. + """ + for where in ("/proc/self/fd", "/dev/fd"): + if Path(where).is_dir(): + return len(list(Path(where).iterdir())) + return None + + +def test_a_thousand_connections_opened_and_closed_leave_nothing_behind( + tmp_path: Path, +) -> None: + """The loop a job runs, a thousand times. + + A descriptor kept per connection is a program that works in a + notebook and dies overnight, and it is invisible until the count is + taken from outside. Eight paths rather than a thousand, because what + is being counted is connections and not files, and reopening the + same database is the shape a job actually has. + """ + before = descriptors() + + for step in range(1000): + conn = zudb.connect(tmp_path / f"cycle{step % 8}.zu1") + conn.execute("INSERT (p:person {uid: 1, name: 'ada'})") + conn.close() + + gc.collect() + if before is not None: + assert descriptors() == before + + # And nothing is holding one open behind the collector's back. + alive = [obj for obj in gc.get_objects() if isinstance(obj, zudb.Connection)] + assert alive == [] + + +def test_a_thousand_connections_dropped_rather_than_closed_leave_nothing_behind( + tmp_path: Path, +) -> None: + """The same loop written by somebody who never calls `close`. + + Which is most people most of the time, so the collector taking a + connection has to release the file the way `close` does. A client + where only the explicit path is clean is a client that leaks for + every caller who used a `for` loop and trusted the language. + """ + before = descriptors() + + for step in range(1000): + conn = zudb.connect(tmp_path / f"dropped{step % 8}.zu1") + del conn + + gc.collect() + if before is not None: + assert descriptors() == before + + +def test_a_connection_closed_with_things_open_on_it_closes_them_too( + tmp_path: Path, +) -> None: + """Close is the one call a caller makes in a `finally`, so it has to + work from every state rather than from the tidy one. + + An appender with rows in it and a transaction that never ended are + the two things that can be open when it arrives. Neither may hold + the file after, and neither may fail the close: a cleanup path that + raises is a cleanup path that hides the error it was cleaning up + after. + """ + before = descriptors() + + conn = zudb.connect(tmp_path / "open.zu1") + conn.execute("INSERT (p:person {uid: 1, name: 'ada'})") + rows = conn.appender("person") + rows.append_row([2, "grace"]) + txn = conn.transaction() + txn.__enter__() + assert conn.in_transaction + + conn.close() + assert conn.closed + + # Both say so rather than reaching through a connection that is + # gone, which is the difference between a message and a segfault. + # Caught with `except` rather than with `pytest.raises`, because the + # traceback that one keeps holds the appender alive past the `del` + # below and its warning would then land in whatever test ran next. + try: + rows.append_row([3, "kay"]) + except zudb.ProgrammingError: + pass + else: + pytest.fail("appending through a closed connection returned normally") + + try: + txn.commit() + except zudb.ProgrammingError: + pass + else: + pytest.fail("committing through a closed connection returned normally") + + # The rows that were buffered went nowhere, which is worth the + # warning it gets: the close was the caller's, and what it threw + # away was the caller's too. + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + del rows, txn + gc.collect() + assert [warning.category for warning in caught] == [ResourceWarning] + + if before is not None: + assert descriptors() == before + + # And the database is a database, opened again by somebody else. + with zudb.connect(tmp_path / "open.zu1") as conn: + assert conn.execute(READ).fetchall() == [(1,)] + + def test_a_statement_that_failed_wrote_nothing_and_left_the_connection_alone( social: zudb.Connection, ) -> None: @@ -363,6 +510,14 @@ def test_the_programs_that_look_like_misuse_and_are_not( # every statement wants. assert len(social.execute(READ, {"unread": 1}).fetchall()) == 3 + # A value nested deeply is not a value nested endlessly. The limit + # that catches a cycle sits where nothing anybody wrote reaches, so + # a list forty deep goes through and comes back. + deep: object = 1 + for _ in range(40): + deep = [deep] + assert social.execute("RETURN $x AS x", {"x": deep}).fetchall()[0][0] is not None + # A label nothing carries matches nothing. A pattern with no answer # is the ordinary answer to a question about a graph, and the other # reading gives a query that fails on the day the last row of a diff --git a/tests/test_register.py b/tests/test_register.py index 06ecc2c..fc4ce5e 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -319,6 +319,7 @@ def test_a_closed_connection_registers_nothing(empty: zudb.Connection) -> None: empty.register("people", {"a": [1]}) +@pytest.mark.timing def test_registering_costs_the_same_whatever_the_frame_holds(empty: zudb.Connection) -> None: """Nothing is copied, so nothing about the call is per row. diff --git a/tests/test_transactions.py b/tests/test_transactions.py index b046c1a..444e85c 100644 --- a/tests/test_transactions.py +++ b/tests/test_transactions.py @@ -205,6 +205,7 @@ def test_a_closed_connection_has_no_transactions(social: zudb.Connection) -> Non assert social.in_transaction is None +@pytest.mark.timing def test_the_wrapper_costs_nothing_worth_measuring(social: zudb.Connection) -> None: """Two statements and a Python object, which is what it should be. diff --git a/tools/valgrind.supp b/tools/valgrind.supp new file mode 100644 index 0000000..18a68a8 --- /dev/null +++ b/tools/valgrind.supp @@ -0,0 +1,150 @@ +# What the leak job does not count, and why. +# +# CPython leaks on purpose. Interned strings, the import machinery's +# tables and a few hundred one-time allocations are never freed, because +# an interpreter that is about to exit has nothing to gain by walking +# them, and the run below reports about eighty of them on a suite that +# does nothing at all. None of that is this client's memory and none of +# it is a bug anybody here can fix. +# +# So the first rule is the only general one, written out once per +# allocator because valgrind matches a function name and C has three of +# them, and once per shape of interpreter because a CPython built +# --enable-shared puts its own code in libpython3.x.so and a CPython +# built without it puts the same code in bin/python3.x. A leak is +# ignored only when the frame that allocated it is inside one of those +# two files. Memory allocated by this extension has _zudb in that frame +# instead and is reported, which is what the job is for. A suppression +# file of the usual shape, a list that grows a line every time somebody +# wants the build green, would suppress the thing being looked for on +# the first bad week; this one cannot, because it names the allocator +# rather than the symptom. +# +# The object patterns are spelled out to the file rather than left as +# */python3*, which is what they used to say. A * in a valgrind object +# pattern matches slashes as well as anything else, so */python3* also +# matched every extension under lib/python3.14/site-packages, this one +# included: a rule meant to name the interpreter was quietly covering +# the code it exists to watch. +# +# The gap it leaves, said out loud: a Python object this extension leaks +# was allocated by the interpreter on its behalf, so it lands under this +# rule. That leak is the one the suite catches from the other side, +# where it counts live zudb.Connection objects after a collection and +# counts the process descriptors before and after a thousand cycles. +# +# The four rules after it are not general. Each one names a library that +# leaks a fixed amount once, at import or at load, and each is here +# because the leak has a name and a reason rather than because it was in +# the way. +# +# Naming a function at all takes symbols, and the wheel a release ships +# is stripped. The leaks job builds one that is not, which is the only +# reason the rules below can say what they mean: a leak report whose +# every frame reads ??? is a report nobody can act on, and a job that +# produces one is a job that gets ignored on the week it finds +# something. + +{ + the interpreter allocating for itself, shared, malloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:malloc + obj:*/libpython3* +} + +{ + the interpreter allocating for itself, shared, realloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:realloc + obj:*/libpython3* +} + +{ + the interpreter allocating for itself, shared, calloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:calloc + obj:*/libpython3* +} + +{ + the interpreter allocating for itself, static, malloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:malloc + obj:*/bin/python3* +} + +{ + the interpreter allocating for itself, static, realloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:realloc + obj:*/bin/python3* +} + +{ + the interpreter allocating for itself, static, calloc + Memcheck:Leak + match-leak-kinds: definite,possible + fun:calloc + obj:*/bin/python3* +} + +# readline builds its keymaps, its terminfo strings and its history +# buffer when the module is initialised and frees none of them, because +# a line editor is live for as long as the process is and there is +# nothing after it to hand them back to. Seventeen records, about a +# hundred and fifty kilobytes, every one of them under PyInit_readline. +# pytest imports readline through its own console handling, so a suite +# that never reads a line still pays for it. +{ + readline's tables, built at import and never freed + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:PyInit_readline +} + +# pyo3 builds each class's member table as a Vec and hands CPython the +# pointer, which is what tp_members is: an array the type holds for as +# long as the type exists, and a type created at import exists until the +# process ends. The Vec is leaked on purpose because freeing it would +# leave the interpreter reading memory that had been handed back. One +# record of eighty bytes, from the one class that has members rather +# than getters, and it is in this extension's object file only because +# pyo3 is compiled into it. +{ + pyo3 handing a type its member table + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:*create_type_object* +} + +# glibc grows the loader's namespace scope array on a dlopen and keeps +# the old one, since a thread walking the list must not have it freed +# underneath it. Every extension import goes through this, so the number +# of records is the number of extensions and the fix is in glibc. +{ + the dynamic loader's scope array, kept on purpose + Memcheck:Leak + match-leak-kinds: definite,possible + ... + fun:_dl_open +} + +# pyarrow builds a tzinfo per timezone name it is asked about and caches +# it for the life of the process. This is the definite half of what +# pyarrow already accounts for four hundred and seventeen possible +# records of at import, and it is the reason the job counts definite +# losses rather than all of them. +{ + pyarrow's timezone cache + Memcheck:Leak + match-leak-kinds: definite,possible + ... + obj:*/libarrow_python.so* +}