From 917b623609ee7425901555364199df8cbece17f9 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Sat, 28 Feb 2026 22:48:30 +0900 Subject: [PATCH 1/3] Introduce TimeoutSeconds utility type for timeout parameters Follow-up refactoring from #7237. Python timeout parameters typically accept both float and int. Several places in the codebase used Either for this, each repeating the same match-and-convert boilerplate. This extracts that into a TimeoutSeconds type in vm::function::number. Refactored sites: - _sqlite3::ConnectArgs.timeout - _thread::AcquireArgs.timeout - _thread::ThreadHandle::join timeout Either in time.rs (6 sites) left unchanged: those are timestamp values with per-branch logic (floor, range checks, etc). Either in select.rs also left unchanged (different type). --- crates/stdlib/src/_sqlite3.rs | 13 +++++-------- crates/vm/src/function/mod.rs | 4 +++- crates/vm/src/function/number.rs | 29 +++++++++++++++++++++++++++++ crates/vm/src/stdlib/thread.rs | 19 +++++++++---------- 4 files changed, 46 insertions(+), 19 deletions(-) diff --git a/crates/stdlib/src/_sqlite3.rs b/crates/stdlib/src/_sqlite3.rs index 971c4ec13ac..375bd1b33b5 100644 --- a/crates/stdlib/src/_sqlite3.rs +++ b/crates/stdlib/src/_sqlite3.rs @@ -61,8 +61,8 @@ mod _sqlite3 { }, convert::IntoObject, function::{ - ArgCallable, ArgIterable, Either, FsPath, FuncArgs, OptionalArg, PyComparisonValue, - PySetterValue, + ArgCallable, ArgIterable, FsPath, FuncArgs, OptionalArg, PyComparisonValue, + PySetterValue, TimeoutSeconds, }, object::{Traverse, TraverseFn}, protocol::{ @@ -333,8 +333,8 @@ mod _sqlite3 { struct ConnectArgs { #[pyarg(any)] database: FsPath, - #[pyarg(any, default = Either::A(5.0))] - timeout: Either, + #[pyarg(any, default = TimeoutSeconds::new(5.0))] + timeout: TimeoutSeconds, #[pyarg(any, default = 0)] detect_types: c_int, #[pyarg(any, default = Some(vm.ctx.empty_str.to_owned()))] @@ -991,10 +991,7 @@ mod _sqlite3 { fn initialize_db(args: &ConnectArgs, vm: &VirtualMachine) -> PyResult { let path = args.database.to_cstring(vm)?; let db = Sqlite::from(SqliteRaw::open(path.as_ptr(), args.uri, vm)?); - let timeout = (match args.timeout { - Either::A(float) => float, - Either::B(int) => int as f64, - } * 1000.0) as c_int; + let timeout = (args.timeout.to_secs_f64() * 1000.0) as c_int; db.busy_timeout(timeout); if let Some(isolation_level) = &args.isolation_level { begin_statement_ptr_from_isolation_level(isolation_level, vm)?; diff --git a/crates/vm/src/function/mod.rs b/crates/vm/src/function/mod.rs index 15048919593..5e70d46e5c7 100644 --- a/crates/vm/src/function/mod.rs +++ b/crates/vm/src/function/mod.rs @@ -21,7 +21,9 @@ pub use fspath::FsPath; pub use getset::PySetterValue; pub(super) use getset::{IntoPyGetterFunc, IntoPySetterFunc, PyGetterFunc, PySetterFunc}; pub use method::{HeapMethodDef, PyMethodDef, PyMethodFlags}; -pub use number::{ArgIndex, ArgIntoBool, ArgIntoComplex, ArgIntoFloat, ArgPrimitiveIndex, ArgSize}; +pub use number::{ + ArgIndex, ArgIntoBool, ArgIntoComplex, ArgIntoFloat, ArgPrimitiveIndex, ArgSize, TimeoutSeconds, +}; pub use protocol::{ArgCallable, ArgIterable, ArgMapping, ArgSequence}; use crate::{PyObject, PyResult, VirtualMachine, builtins::PyStr, convert::TryFromBorrowedObject}; diff --git a/crates/vm/src/function/number.rs b/crates/vm/src/function/number.rs index b53208bcd93..280ff9bcf3d 100644 --- a/crates/vm/src/function/number.rs +++ b/crates/vm/src/function/number.rs @@ -196,3 +196,32 @@ impl From for isize { arg.value } } + +/// A Python timeout value that accepts both `float` and `int`. +/// +/// `TimeoutSeconds` implements `FromArgs` so that a built-in function can accept +/// timeout parameters given as either `float` or `int`, normalizing them to `f64`. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct TimeoutSeconds { + value: f64, +} + +impl TimeoutSeconds { + pub const fn new(secs: f64) -> Self { + Self { value: secs } + } + + #[inline] + pub fn to_secs_f64(self) -> f64 { + self.value + } +} + +impl TryFromObject for TimeoutSeconds { + fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { + super::Either::::try_from_object(vm, obj).map(|e| match e { + super::Either::A(f) => Self { value: f }, + super::Either::B(i) => Self { value: i as f64 }, + }) + } +} diff --git a/crates/vm/src/stdlib/thread.rs b/crates/vm/src/stdlib/thread.rs index 21b19fb7560..505209d84b2 100644 --- a/crates/vm/src/stdlib/thread.rs +++ b/crates/vm/src/stdlib/thread.rs @@ -15,7 +15,7 @@ pub(crate) mod _thread { builtins::{PyDictRef, PyStr, PyTupleRef, PyType, PyTypeRef, PyUtf8StrRef}, common::wtf8::Wtf8Buf, frame::FrameRef, - function::{ArgCallable, Either, FuncArgs, KwArgs, OptionalArg, PySetterValue}, + function::{ArgCallable, FuncArgs, KwArgs, OptionalArg, PySetterValue, TimeoutSeconds}, types::{Constructor, GetAttr, Representable, SetAttr}, }; use alloc::{ @@ -65,17 +65,14 @@ pub(crate) mod _thread { struct AcquireArgs { #[pyarg(any, default = true)] blocking: bool, - #[pyarg(any, default = Either::A(-1.0))] - timeout: Either, + #[pyarg(any, default = TimeoutSeconds::new(-1.0))] + timeout: TimeoutSeconds, } macro_rules! acquire_lock_impl { ($mu:expr, $args:expr, $vm:expr) => {{ let (mu, args, vm) = ($mu, $args, $vm); - let timeout = match args.timeout { - Either::A(f) => f, - Either::B(i) => i as f64, - }; + let timeout = args.timeout.to_secs_f64(); match args.blocking { true if timeout == -1.0 => { mu.lock(); @@ -1092,13 +1089,15 @@ pub(crate) mod _thread { #[pymethod] fn join( &self, - timeout: OptionalArg>>, + timeout: OptionalArg>, vm: &VirtualMachine, ) -> PyResult<()> { // Convert timeout to Duration (None or negative = infinite wait) let timeout_duration = match timeout.flatten() { - Some(Either::A(t)) if t >= 0.0 => Some(Duration::from_secs_f64(t)), - Some(Either::B(t)) if t >= 0 => Some(Duration::from_secs(t as u64)), + Some(t) => { + let secs = t.to_secs_f64(); + (secs >= 0.0).then(|| Duration::from_secs_f64(secs)) + } _ => None, }; From e04f3e268b1378a262abc43c7a3afe66933d8ef3 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Sat, 28 Feb 2026 23:12:54 +0900 Subject: [PATCH 2/3] Validate timeout in ThreadHandle::join to prevent panic --- crates/vm/src/function/number.rs | 12 ++++++++---- crates/vm/src/stdlib/thread.rs | 20 +++++++++++++------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/crates/vm/src/function/number.rs b/crates/vm/src/function/number.rs index 280ff9bcf3d..7d0431f62fe 100644 --- a/crates/vm/src/function/number.rs +++ b/crates/vm/src/function/number.rs @@ -219,9 +219,13 @@ impl TimeoutSeconds { impl TryFromObject for TimeoutSeconds { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { - super::Either::::try_from_object(vm, obj).map(|e| match e { - super::Either::A(f) => Self { value: f }, - super::Either::B(i) => Self { value: i as f64 }, - }) + let value = match super::Either::::try_from_object(vm, obj)? { + super::Either::A(f) => f, + super::Either::B(i) => i as f64, + }; + if value.is_nan() { + return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned())); + } + Ok(Self { value }) } } diff --git a/crates/vm/src/stdlib/thread.rs b/crates/vm/src/stdlib/thread.rs index 505209d84b2..67e49cbaece 100644 --- a/crates/vm/src/stdlib/thread.rs +++ b/crates/vm/src/stdlib/thread.rs @@ -79,15 +79,12 @@ pub(crate) mod _thread { Ok(true) } true if timeout < 0.0 => { - Err(vm.new_value_error("timeout value must be positive".to_owned())) + Err(vm.new_value_error("timeout value must be a non-negative number".to_owned())) } true => { - // modified from std::time::Duration::from_secs_f64 to avoid a panic. - // TODO: put this in the Duration::try_from_object impl, maybe? - let nanos = timeout * 1_000_000_000.0; - if timeout > TIMEOUT_MAX as f64 || nanos < 0.0 || !nanos.is_finite() { + if timeout > TIMEOUT_MAX { return Err(vm.new_overflow_error( - "timestamp too large to convert to Rust Duration".to_owned(), + "timeout value is too large".to_owned(), )); } @@ -1096,7 +1093,16 @@ pub(crate) mod _thread { let timeout_duration = match timeout.flatten() { Some(t) => { let secs = t.to_secs_f64(); - (secs >= 0.0).then(|| Duration::from_secs_f64(secs)) + if secs >= 0.0 { + if secs > TIMEOUT_MAX { + return Err(vm.new_overflow_error( + "timeout value is too large".to_owned(), + )); + } + Some(Duration::from_secs_f64(secs)) + } else { + None + } } _ => None, }; From aa5a6f0cfbdb491623abb63fad6c5a4c3ae05aca Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Sun, 1 Mar 2026 23:21:26 +0900 Subject: [PATCH 3/3] refactor: move TimeoutSeconds from number to time module --- crates/vm/src/function/mod.rs | 6 +++--- crates/vm/src/function/number.rs | 33 ------------------------------- crates/vm/src/function/time.rs | 34 ++++++++++++++++++++++++++++++++ crates/vm/src/stdlib/thread.rs | 13 ++++++------ 4 files changed, 43 insertions(+), 43 deletions(-) create mode 100644 crates/vm/src/function/time.rs diff --git a/crates/vm/src/function/mod.rs b/crates/vm/src/function/mod.rs index 5e70d46e5c7..4be94e3f0be 100644 --- a/crates/vm/src/function/mod.rs +++ b/crates/vm/src/function/mod.rs @@ -8,6 +8,7 @@ mod getset; mod method; mod number; mod protocol; +mod time; pub use argument::{ ArgumentError, FromArgOptional, FromArgs, FuncArgs, IntoFuncArgs, KwArgs, OptionalArg, @@ -21,10 +22,9 @@ pub use fspath::FsPath; pub use getset::PySetterValue; pub(super) use getset::{IntoPyGetterFunc, IntoPySetterFunc, PyGetterFunc, PySetterFunc}; pub use method::{HeapMethodDef, PyMethodDef, PyMethodFlags}; -pub use number::{ - ArgIndex, ArgIntoBool, ArgIntoComplex, ArgIntoFloat, ArgPrimitiveIndex, ArgSize, TimeoutSeconds, -}; +pub use number::{ArgIndex, ArgIntoBool, ArgIntoComplex, ArgIntoFloat, ArgPrimitiveIndex, ArgSize}; pub use protocol::{ArgCallable, ArgIterable, ArgMapping, ArgSequence}; +pub use time::TimeoutSeconds; use crate::{PyObject, PyResult, VirtualMachine, builtins::PyStr, convert::TryFromBorrowedObject}; use builtin::{BorrowedParam, OwnedParam, RefParam}; diff --git a/crates/vm/src/function/number.rs b/crates/vm/src/function/number.rs index 7d0431f62fe..b53208bcd93 100644 --- a/crates/vm/src/function/number.rs +++ b/crates/vm/src/function/number.rs @@ -196,36 +196,3 @@ impl From for isize { arg.value } } - -/// A Python timeout value that accepts both `float` and `int`. -/// -/// `TimeoutSeconds` implements `FromArgs` so that a built-in function can accept -/// timeout parameters given as either `float` or `int`, normalizing them to `f64`. -#[derive(Debug, Clone, Copy, PartialEq)] -pub struct TimeoutSeconds { - value: f64, -} - -impl TimeoutSeconds { - pub const fn new(secs: f64) -> Self { - Self { value: secs } - } - - #[inline] - pub fn to_secs_f64(self) -> f64 { - self.value - } -} - -impl TryFromObject for TimeoutSeconds { - fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { - let value = match super::Either::::try_from_object(vm, obj)? { - super::Either::A(f) => f, - super::Either::B(i) => i as f64, - }; - if value.is_nan() { - return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned())); - } - Ok(Self { value }) - } -} diff --git a/crates/vm/src/function/time.rs b/crates/vm/src/function/time.rs new file mode 100644 index 00000000000..29f14495d14 --- /dev/null +++ b/crates/vm/src/function/time.rs @@ -0,0 +1,34 @@ +use crate::{PyObjectRef, PyResult, TryFromObject, VirtualMachine}; + +/// A Python timeout value that accepts both `float` and `int`. +/// +/// `TimeoutSeconds` implements `FromArgs` so that a built-in function can accept +/// timeout parameters given as either `float` or `int`, normalizing them to `f64`. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct TimeoutSeconds { + value: f64, +} + +impl TimeoutSeconds { + pub const fn new(secs: f64) -> Self { + Self { value: secs } + } + + #[inline] + pub fn to_secs_f64(self) -> f64 { + self.value + } +} + +impl TryFromObject for TimeoutSeconds { + fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { + let value = match super::Either::::try_from_object(vm, obj)? { + super::Either::A(f) => f, + super::Either::B(i) => i as f64, + }; + if value.is_nan() { + return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned())); + } + Ok(Self { value }) + } +} diff --git a/crates/vm/src/stdlib/thread.rs b/crates/vm/src/stdlib/thread.rs index 67e49cbaece..5223a97b0c3 100644 --- a/crates/vm/src/stdlib/thread.rs +++ b/crates/vm/src/stdlib/thread.rs @@ -79,13 +79,12 @@ pub(crate) mod _thread { Ok(true) } true if timeout < 0.0 => { - Err(vm.new_value_error("timeout value must be a non-negative number".to_owned())) + Err(vm + .new_value_error("timeout value must be a non-negative number".to_owned())) } true => { if timeout > TIMEOUT_MAX { - return Err(vm.new_overflow_error( - "timeout value is too large".to_owned(), - )); + return Err(vm.new_overflow_error("timeout value is too large".to_owned())); } Ok(mu.try_lock_for(Duration::from_secs_f64(timeout))) @@ -1095,9 +1094,9 @@ pub(crate) mod _thread { let secs = t.to_secs_f64(); if secs >= 0.0 { if secs > TIMEOUT_MAX { - return Err(vm.new_overflow_error( - "timeout value is too large".to_owned(), - )); + return Err( + vm.new_overflow_error("timeout value is too large".to_owned()) + ); } Some(Duration::from_secs_f64(secs)) } else {