From 7efa4ce2ac3c37cdc7bcfb4c58824ebe56ce0b0c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 16 Jul 2026 15:29:01 +0000 Subject: [PATCH] Simplify the logcontext internals - switch_context dispatched on a stringly-typed method name with a catch-all arm that silently meant 'stop'; use a two-variant enum matched exhaustively on both the native and Python-dispatch paths (interning the method name for the latter). - none_to_option re-implemented what pyo3's extract::>() does natively; use the extract idiom the codebase already uses elsewhere. - cputime_delta returned PyResult but its body is infallible; return the tuple directly and drop the ? plumbing at the call sites. - registered_sentinel in the Rust tests was a pass-through wrapper around sentinel() with a misleading name; call sentinel() directly. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JFbRtswu7rsHrttJFauUUb --- rust/src/logging/context.rs | 61 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/rust/src/logging/context.rs b/rust/src/logging/context.rs index 25dc210035..1161e4aa3f 100644 --- a/rust/src/logging/context.rs +++ b/rust/src/logging/context.rs @@ -345,15 +345,6 @@ fn get_thread_id(py: Python<'_>) -> PyResult { }) } -/// Normalise a Python value to `None` if it is `None`, else `Some`. -fn none_to_option(obj: Bound<'_, PyAny>) -> Option> { - if obj.is_none() { - None - } else { - Some(obj.unbind()) - } -} - /// Propagate a usage update to the parent context, if there is a (truthy) one. /// /// A plain base `LoggingContext` parent — the overwhelmingly common case, e.g. @@ -424,7 +415,7 @@ fn get_thread_rusage() -> Option<(f64, f64)> { /// /// Mirrors the former `_get_cputime`: guards against the clock going backwards /// (clamping to zero and logging, as the accounting must never go negative). -fn cputime_delta(current: (f64, f64), start: (f64, f64)) -> PyResult<(f64, f64)> { +fn cputime_delta(current: (f64, f64), start: (f64, f64)) -> (f64, f64) { let mut utime_delta = current.0 - start.0; let mut stime_delta = current.1 - start.1; @@ -438,7 +429,7 @@ fn cputime_delta(current: (f64, f64), start: (f64, f64)) -> PyResult<(f64, f64)> stime_delta = 0.0; } - Ok((utime_delta, stime_delta)) + (utime_delta, stime_delta) } /// Additional context for log formatting, tracking which request a unit of work @@ -568,9 +559,9 @@ impl LoggingContext { if let Some(parent) = &parent_context { let parent = parent.bind(py); // which request this corresponds to - self.request = none_to_option(parent.getattr("request")?); + self.request = parent.getattr("request")?.extract()?; // we also track the current scope - self.scope = none_to_option(parent.getattr("scope")?); + self.scope = parent.getattr("scope")?.extract()?; } if let Some(request) = request { @@ -721,7 +712,7 @@ impl LoggingContext { if let Some(start) = usage_start { if get_thread_id(py)? == main_thread { if let Some(current) = get_thread_rusage() { - let (utime_delta, stime_delta) = cputime_delta(current, start)?; + let (utime_delta, stime_delta) = cputime_delta(current, start); res.ru_utime += utime_delta; res.ru_stime += stime_delta; } @@ -907,7 +898,7 @@ impl LoggingContext { return Ok(()); }; - let (utime_delta, stime_delta) = cputime_delta(current, start)?; + let (utime_delta, stime_delta) = cputime_delta(current, start); slf.borrow().add_cputime(py, utime_delta, stime_delta)?; Ok(()) })(); @@ -917,6 +908,13 @@ impl LoggingContext { } } +/// Which way a [`switch_context`] dispatch goes. +#[derive(Clone, Copy)] +enum SwitchDirection { + Start, + Stop, +} + /// Dispatch a `stop`/`start` to a context during a switch, respecting subclass /// overrides. /// @@ -925,21 +923,26 @@ impl LoggingContext { /// no-op. Anything else is a Python subclass (e.g. /// `BackgroundProcessLoggingContext`), so we go through Python — materialising /// the rusage as a `(utime, stime)` tuple only here, off the hot path — so its -/// overrides run. +/// overrides run. Both arms match on the same [`SwitchDirection`], so the +/// native and Python paths provably dispatch the same operation. fn switch_context( obj: &Bound<'_, PyAny>, - method: &str, + direction: SwitchDirection, rusage: Option<(f64, f64)>, ) -> PyResult<()> { let py = obj.py(); if let Ok(base) = obj.cast_exact::() { - match method { - "start" => LoggingContext::start_inner(base, rusage), - _ => LoggingContext::stop_inner(base, rusage), + match direction { + SwitchDirection::Start => LoggingContext::start_inner(base, rusage), + SwitchDirection::Stop => LoggingContext::stop_inner(base, rusage), } } else if obj.is(sentinel(py).bind(py)) { Ok(()) } else { + let method = match direction { + SwitchDirection::Start => intern!(py, "start"), + SwitchDirection::Stop => intern!(py, "stop"), + }; obj.call_method1(method, (rusage,))?; Ok(()) } @@ -965,11 +968,11 @@ pub fn set_current_context(py: Python<'_>, context: Bound<'_, PyAny>) -> PyResul if !current.bind(py).is(&context) { let rusage = get_thread_rusage(); - switch_context(current.bind(py), "stop", rusage)?; + switch_context(current.bind(py), SwitchDirection::Stop, rusage)?; // Raw slot write; we already hold `current`, so ignore the previous value // it returns. swap_current_context(py, context.clone().unbind()); - switch_context(&context, "start", rusage)?; + switch_context(&context, SwitchDirection::Start, rusage)?; } Ok(current) @@ -1075,17 +1078,11 @@ mod tests { use super::*; - /// The native sentinel singleton (created lazily on first use), used for - /// identity assertions. - fn registered_sentinel(py: Python<'_>) -> Py { - sentinel(py).clone_ref(py) - } - #[test] fn thread_local_defaults_to_sentinel() { Python::initialize(); Python::attach(|py| { - let sentinel = registered_sentinel(py); + let sentinel = sentinel(py); // Nothing set on this (fresh) test thread → sentinel. assert!(current_context(py).bind(py).is(sentinel.bind(py))); }); @@ -1095,7 +1092,7 @@ mod tests { fn swap_returns_previous_and_updates_thread_local() { Python::initialize(); Python::attach(|py| { - let sentinel = registered_sentinel(py); + let sentinel = sentinel(py); let a = PyString::new(py, "A").into_any().unbind(); let b = PyString::new(py, "B").into_any().unbind(); @@ -1111,7 +1108,7 @@ mod tests { // Restore the sentinel so we don't leak into any other test that // happens to reuse this OS thread from the test harness pool. - swap_current_context(py, sentinel); + swap_current_context(py, sentinel.clone_ref(py)); }); } @@ -1119,7 +1116,7 @@ mod tests { fn task_local_takes_precedence_over_thread_local() { Python::initialize(); Python::attach(|py| { - let sentinel = registered_sentinel(py); + let sentinel = sentinel(py); let task_ctx = PyString::new(py, "TASKCTX").into_any().unbind(); // Outside any scoped task, `current_context` resolves the