From a1541bb3a4f64adec267fba31cc5d8b0cedfc4fd Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 5 Aug 2026 17:10:18 +0100 Subject: [PATCH] Improve comments --- rust/src/database/mod.rs | 6 ++-- rust/src/database/postgres/helpers.rs | 50 +++++++++++---------------- rust/src/database/postgres/mod.rs | 24 +++++-------- rust/src/database/postgres/value.rs | 36 ++++++++----------- rust/src/tokio_runtime.rs | 16 ++++----- 5 files changed, 53 insertions(+), 79 deletions(-) diff --git a/rust/src/database/mod.rs b/rust/src/database/mod.rs index 42b4bb437a..759966025a 100644 --- a/rust/src/database/mod.rs +++ b/rust/src/database/mod.rs @@ -1,4 +1,4 @@ -//! DBAPI2-shaped Connection / Cursor types implemented in Rust. +//! Database access implemented in Rust. //! //! Currently this provides a single Postgres backend ([`tokio_postgres`]); see //! the [`postgres`] submodule. @@ -17,8 +17,8 @@ pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> m.add_submodule(&child)?; - // Mirror the convention used by other rust submodules so - // `from synapse.synapse_rust import database` works. + // We need to manually add the module to sys.modules to make `from + // synapse.synapse_rust import database` work. py.import("sys")? .getattr("modules")? .set_item("synapse.synapse_rust.database", child)?; diff --git a/rust/src/database/postgres/helpers.rs b/rust/src/database/postgres/helpers.rs index b44c155dd1..3b651de71b 100644 --- a/rust/src/database/postgres/helpers.rs +++ b/rust/src/database/postgres/helpers.rs @@ -14,7 +14,7 @@ */ //! Extension traits for driving [`tokio_postgres`] futures to completion from -//! the synchronous, GIL-holding Python methods. +//! synchronous, GIL-holding Python code. //! //! Both [`BlockingPostgres`] and [`BlockingPostgresStream`] release the GIL //! (`py.detach`) while blocking on the shared tokio runtime, so other Python @@ -23,10 +23,9 @@ //! the `detach` boundary. //! //! The stream helper is generic over the underlying stream type rather than -//! hard-wired to [`tokio_postgres::RowStream`]. In production it is always used -//! with a `RowStream`, but keeping it generic lets the cursor state machine -//! (which uses these helpers) be unit-tested against an in-memory fake stream -//! with no live database — see [`BlockingPostgresStream`]'s tests. +//! hard-wired to [`tokio_postgres::RowStream`], so code built on it can be +//! unit-tested against an in-memory stream with no live database — see +//! [`BlockingPostgresStream`]'s tests. use std::{future::Future, pin::Pin}; @@ -52,8 +51,8 @@ where /// (see [`crate::tokio_runtime`]) entered first with /// [`Handle::enter`](tokio::runtime::Handle::enter); [`Handle::current`] /// panics otherwise. The blocking wait runs on the calling (Python) thread, - /// never on a runtime worker — see this module's docs for why that can't - /// deadlock. + /// never on a runtime worker, so it cannot starve the worker threads that + /// complete the future. fn block_on(self, py: Python<'_>) -> Self::Output { py.detach(|| Handle::current().block_on(self)) } @@ -91,16 +90,13 @@ where /// the shared runtime only when the next item isn't already buffered. /// /// Implemented for any pinned, fused stream (`Pin<&mut Fuse>`) whose items -/// can cross the GIL-release boundary. In production `S` is -/// [`tokio_postgres::RowStream`]; the generic bound is what lets the cursor -/// logic be tested against an in-memory fake. +/// can cross the GIL-release boundary, so it can be tested against an +/// in-memory stream as well as a [`tokio_postgres::RowStream`]. /// -/// The [`Fuse`] is *required* by the impl (the trait is implemented only for -/// `Pin<&mut Fuse>`), not merely assumed. This matters because -/// [`Self::get_next_if_ready`] may poll the stream again after it has finished: -/// a bare `Stream` is free to panic if polled past completion, whereas a fused -/// stream simply keeps yielding `None`. So repeated `get_next_if_ready` / -/// `block_on_next` calls after exhaustion are safe by construction. +/// The [`Fuse`] bound matters because [`Self::get_next_if_ready`] may poll the +/// stream again after it has finished. A bare `Stream` is allowed to panic if +/// polled past completion; a fused stream keeps yielding `None`, so calls +/// after exhaustion are safe. pub trait BlockingPostgresStream where Self: futures::Stream + Sized + Send + Ungil + Unpin, @@ -134,9 +130,8 @@ where } } -// Blanket impl over any pinned, fused stream. Requiring the helper bounds here -// (rather than only for `RowStream`) is what makes the cursor logic testable -// with a fake stream. +// Blanket impl over any pinned, fused stream, not just `RowStream`, so the +// tests can use an in-memory stream. impl BlockingPostgresStream for Pin<&mut Fuse> where Self: futures::Stream + Send + Ungil + Unpin, @@ -156,11 +151,9 @@ mod tests { use super::*; - /// A throwaway runtime standing in for the shared one. Production code - /// enters `crate::tokio_runtime`'s runtime on each thread; the helpers only - /// need *some* runtime entered on the current thread (so [`Handle::current`] - /// resolves), and (as in production) the blocking wait runs on this test - /// thread rather than on a worker. Each test calls `rt.enter()` and holds + /// A throwaway runtime standing in for the shared one. The helpers only + /// need *some* runtime entered on the current thread, so that + /// [`Handle::current`] resolves. Each test calls `rt.enter()` and holds /// the guard for the duration. fn test_runtime() -> Runtime { tokio::runtime::Builder::new_multi_thread() @@ -189,8 +182,8 @@ mod tests { let ok = async { Ok::(5) }; assert_eq!(ok.block_on_result(py).unwrap(), 5); // The error path (mapping a `tokio_postgres::Error` to a `PyErr`) - // can't be unit-tested here, as that error type can't be - // constructed by hand; it's exercised by the integration tests. + // isn't covered here, because that error type can't be constructed + // by hand. Exercising it needs a live server. }); } @@ -246,9 +239,8 @@ mod tests { assert_eq!(stream.as_mut().block_on_next(py), Some(Ok(7))); assert_eq!(stream.as_mut().block_on_next(py), None); - // And, on a fresh stream, `block_on_next` handles the pending first - // poll entirely on its own (no preceding `get_next_if_ready`), - // proving it doesn't rely on being "primed" by an earlier call. + // And, on a fresh stream, `block_on_next` handles a pending first + // poll on its own, with no preceding `get_next_if_ready`. let stream = stream::once(async { tokio::task::yield_now().await; Ok::(8) diff --git a/rust/src/database/postgres/mod.rs b/rust/src/database/postgres/mod.rs index 3710f8b972..dfdfa31532 100644 --- a/rust/src/database/postgres/mod.rs +++ b/rust/src/database/postgres/mod.rs @@ -1,29 +1,21 @@ //! [`tokio_postgres`]-backed Postgres backend for the Rust `database` module. //! -//! This module will grow the Python-facing `Connection` / `Cursor` classes and -//! the `connect` factory; for now it hosts the value-mapping layer ([`value`]) -//! that converts between Python objects and Postgres' binary wire format. -//! -//! The driver itself is async; the eventual `Connection` / `Cursor` types will -//! drive it from sync Python methods via a shared multi-thread tokio runtime. +//! The driver is async. [`helpers`] drives its futures to completion from +//! synchronous Python code on the shared tokio runtime, and [`value`] converts +//! between Python objects and Postgres' binary wire format. use pyo3::exceptions::PyRuntimeError; use pyo3::prelude::*; use pyo3::types::PyModule; -// `pub` (rather than private) so the not-yet-consumed public items in these -// submodules are reachable from the crate root as public API. This is what -// stops clippy's `dead_code` lint from firing on them before the -// cursor/connection code (added in later changes) wires them up; the visibility -// is tightened back to private once that happens. +// `pub` so the items in these submodules count as public API even though +// nothing consumes them yet, which keeps clippy's `dead_code` lint quiet. +// Tighten to private once the connection/cursor code uses them. pub mod helpers; pub mod value; -/// Register the `postgres` submodule under the parent `database` module. -/// -/// The `Connection` / `Cursor` classes and the `connect` factory are added in -/// later changes; for now this just creates the (otherwise empty) submodule so -/// the module tree — and the `value` mapping layer hanging off it — exists. +/// Register the (currently empty) `postgres` submodule under the parent +/// `database` module. pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { let child = PyModule::new(py, "postgres")?; diff --git a/rust/src/database/postgres/value.rs b/rust/src/database/postgres/value.rs index 01f0868e61..ada8701918 100644 --- a/rust/src/database/postgres/value.rs +++ b/rust/src/database/postgres/value.rs @@ -1,12 +1,9 @@ //! Conversions between Python values and the Postgres SQL value //! representations. //! -//! Kept in its own module so the cursor code stays focused on the DBAPI shape -//! rather than the type-mapping table. -//! -//! First cut: int / float / bool / str / bytes / None. Lists (for -//! `ANY($1)`-style queries) and richer types — json, decimal, timestamps — -//! are deferred to a follow-up. +//! Supports int / float / bool / str / bytes / None. Lists (for +//! `ANY($1)`-style queries) and richer types such as json, decimal and +//! timestamps are not yet supported. //! //! The mapping is column-type-driven on the way *out* (a single Python `int` //! becomes `INT2`/`INT4`/`INT8` depending on the column it is bound to) and @@ -52,14 +49,12 @@ impl PgValue { /// Classify a Python object into a [`PgValue`], or error if its type isn't /// one we know how to send to Postgres. /// - /// Two subtleties worth calling out: /// * `bool` is classified as [`PgValue::Bool`], never [`PgValue::Int`], /// even though Python's `bool` is a subclass of `int`. /// * `int` must fit in an `i64`; a larger Python integer raises an /// `OverflowError` here, since Postgres has no wider integer type in /// this mapping. - /// - /// A type we don't recognise raises `TypeError`. + /// * A type we don't recognise raises `TypeError`. pub fn from_py(obj: &Bound) -> PyResult { if obj.is_none() { return Ok(PgValue::Null); @@ -88,8 +83,8 @@ impl PgValue { } } -// Lets PyO3 extract a `PgValue` directly from a Python argument, e.g. when a -// cursor method takes `Option>` for its parameters. +// Lets PyO3 extract a `PgValue` directly from a Python argument, e.g. from a +// method that takes `Option>` as its parameter list. impl<'a, 'py> FromPyObject<'a, 'py> for PgValue { type Error = PyErr; @@ -135,13 +130,10 @@ impl ToSql for PgValue { Ok(IsNull::No) } (&PgValue::Float(v), &Type::FLOAT4) => { - // The `as` cast here generates the closest f32 to the f64, - // with loss of precision. Since Python floats are variable - // precision anyway, this is the best we can do. - // - // (Crucially, there is no way of doing a "fallible" cast - // here, since unlike integers there is no notion of "out of - // range" for floats, just varying precision.) + // The `as` cast narrows to the nearest f32, losing precision. + // Unlike the integer arms there is no fallible conversion to + // use, since floats have no notion of "out of range", just + // varying precision. float4_to_sql(v as f32, buf); Ok(IsNull::No) } @@ -359,8 +351,8 @@ mod tests { #[test] fn from_py_extracts_via_frompyobject() { - // The cursor binds parameters by extracting `PgValue` straight off the - // Python argument; check that `FromPyObject` path forwards to `from_py`. + // Check that extracting a `PgValue` from a Python object (the + // `FromPyObject` path) forwards to `from_py`. Python::initialize(); Python::attach(|py| { let obj = 7i64.into_pyobject(py).unwrap().into_any(); @@ -376,7 +368,7 @@ mod tests { let list = pyo3::types::PyList::new(py, [1, 2, 3]).unwrap(); let err = PgValue::from_py(&list.into_any()).unwrap_err(); assert!(err.is_instance_of::(py)); - // The message names the offending type, which is the useful part. + // The message should name the offending type. assert!(err.to_string().contains("list"), "got: {err}"); }); } @@ -420,7 +412,7 @@ mod tests { encode(&PgValue::Float(value), &Type::FLOAT4).0, (value as f32).to_be_bytes() ); - // And the result is provably narrower than the FLOAT8 encoding. + // And the bytes are not just a truncation of the f64 encoding. assert_ne!( encode(&PgValue::Float(value), &Type::FLOAT4).0, value.to_be_bytes()[..4].to_vec() diff --git a/rust/src/tokio_runtime.rs b/rust/src/tokio_runtime.rs index d1338ed831..f7c1c0de7e 100644 --- a/rust/src/tokio_runtime.rs +++ b/rust/src/tokio_runtime.rs @@ -51,10 +51,9 @@ impl PyTokioRuntime { impl PyTokioRuntime { /// Build the runtime if it hasn't been built yet. /// - /// Idempotent, so it is safe to call both from the reactor's - /// `callWhenRunning(start)` hook and from a caller that needs the runtime - /// before the reactor has run that hook (see [`runtime_handle`]): whichever - /// runs first builds it, the other is a no-op. + /// Both the reactor's `callWhenRunning(start)` hook and [`runtime_handle`] + /// call this, in either order. Whichever runs first builds the runtime; + /// for the other it is a no-op. fn ensure_started(&mut self) -> PyResult<()> { if self.runtime.is_some() { return Ok(()); @@ -92,11 +91,10 @@ pub fn runtime<'a>(reactor: &Bound<'a, PyAny>) -> PyResult) -> PyResult { let runtime = get_or_install(reactor)?; let mut runtime = runtime.borrow_mut();