Skip to content

Introduce TimeoutSeconds utility type for timeout parameters #7271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ever0de wants to merge 3 commits into
base: main
Choose a base branch
from
+63 −26
Open

Introduce TimeoutSeconds utility type for timeout parameters #7271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
917b623
Introduce TimeoutSeconds utility type for timeout parameters
ever0de Feb 28, 2026
e04f3e2
Validate timeout in ThreadHandle::join to prevent panic
ever0de Feb 28, 2026
aa5a6f0
refactor: move TimeoutSeconds from number to time module
ever0de Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions crates/stdlib/src/_sqlite3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -333,8 +333,8 @@ mod _sqlite3 {
struct ConnectArgs {
#[pyarg(any)]
database: FsPath,
#[pyarg(any, default = Either::A(5.0))]
timeout: Either<f64, i64>,
#[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()))]
Expand Down Expand Up @@ -991,10 +991,7 @@ mod _sqlite3 {
fn initialize_db(args: &ConnectArgs, vm: &VirtualMachine) -> PyResult<Sqlite> {
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)?;
Expand Down
2 changes: 2 additions & 0 deletions crates/vm/src/function/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod getset;
mod method;
mod number;
mod protocol;
mod time;

pub use argument::{
ArgumentError, FromArgOptional, FromArgs, FuncArgs, IntoFuncArgs, KwArgs, OptionalArg,
Expand All @@ -23,6 +24,7 @@ pub(super) use getset::{IntoPyGetterFunc, IntoPySetterFunc, PyGetterFunc, PySett
pub use method::{HeapMethodDef, PyMethodDef, PyMethodFlags};
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};
Expand Down
34 changes: 34 additions & 0 deletions crates/vm/src/function/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Self> {
let value = match super::Either::<f64, i64>::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 })
}
}
40 changes: 22 additions & 18 deletions crates/vm/src/stdlib/thread.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -65,33 +65,26 @@ pub(crate) mod _thread {
struct AcquireArgs {
#[pyarg(any, default = true)]
blocking: bool,
#[pyarg(any, default = Either::A(-1.0))]
timeout: Either<f64, i64>,
#[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();
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() {
return Err(vm.new_overflow_error(
"timestamp too large to convert to Rust Duration".to_owned(),
));
if timeout > TIMEOUT_MAX {
return Err(vm.new_overflow_error("timeout value is too large".to_owned()));
}

Ok(mu.try_lock_for(Duration::from_secs_f64(timeout)))
Expand Down Expand Up @@ -1092,13 +1085,24 @@ pub(crate) mod _thread {
#[pymethod]
fn join(
&self,
timeout: OptionalArg<Option<Either<f64, i64>>>,
timeout: OptionalArg<Option<TimeoutSeconds>>,
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();
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
}
}
Comment on lines +1093 to +1105
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

NaN handling is inconsistent with acquire_lock_impl!.

The condition secs >= 0.0 (line 1099) evaluates to false for NaN, causing NaN inputs to silently become infinite waits (via the else branch). In contrast, acquire_lock_impl! (lines 84–92) routes NaN through the validation path where !nanos.is_finite() correctly raises an OverflowError.

To match acquire_lock_impl! semantics and the intent of the previous fix, invert the condition so NaN reaches the validation logic:

Proposed fix 
             Some(t) => {
                 let secs = t.to_secs_f64();
-                if secs >= 0.0 {
+                if secs < 0.0 {
+                    None
+                } else {
                     let nanos = secs * 1_000_000_000.0;
                     if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() {
                         return Err(vm.new_overflow_error(
                             "timestamp too large to convert to Rust Duration".to_owned(),
                         ));
                     }
                     Some(Duration::from_secs_f64(secs))
-                } else {
-                    None
                 }
             }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Some(t) => {
let secs = t.to_secs_f64();
if secs >= 0.0 {
let nanos = secs * 1_000_000_000.0;
if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() {
return Err(vm.new_overflow_error(
"timestamp too large to convert to Rust Duration".to_owned(),
));
}
Some(Duration::from_secs_f64(secs))
} else {
None
}
}
Some(t) => {
let secs = t.to_secs_f64();
if secs < 0.0 {
None
} else {
let nanos = secs * 1_000_000_000.0;
if secs > TIMEOUT_MAX || nanos < 0.0 || !nanos.is_finite() {
return Err(vm.new_overflow_error(
"timestamp too large to convert to Rust Duration".to_owned(),
));
}
Some(Duration::from_secs_f64(secs))
}
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/stdlib/thread.rs` around lines 1097 - 1110, The branch treating
Sec as non-negative currently uses `if secs >= 0.0 { ... } else { None }`, which
makes NaN fall into the `else` and become an infinite wait; invert the condition
so NaN goes through the validation path used by acquire_lock_impl!. Replace the
`if secs >= 0.0 { ... } else { None }` pattern with the inverse `if secs < 0.0 {
None } else { ... }`, keeping the inner validation that checks `nanos`,
`TIMEOUT_MAX`, `!nanos.is_finite()` and calls `vm.new_overflow_error(...)` or
`Duration::from_secs_f64(secs)` as before.

Copy link
Contributor Author

@ever0de ever0de Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already addressed — TimeoutSeconds::try_from_object() rejects NaN with ValueError("Invalid value NaN (not a number)") before join() is ever called, so NaN cannot reach this code path.

Copy link
Contributor

@coderabbitai coderabbitai bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the implementation of TimeoutSeconds in number.rs to see if NaN is rejected
cat crates/vm/src/function/number.rs | grep -A 30 "impl TryFromObject for TimeoutSeconds"

Length of output: 460

@ever0de, you're absolutely right, and I apologize for the incorrect review comment.

The TimeoutSeconds::try_from_object() implementation explicitly validates and rejects NaN values:

if value.is_nan() {
    return Err(vm.new_value_error("Invalid value NaN (not a number)".to_owned()));
}

This means NaN is caught during the conversion from Python object to TimeoutSeconds, so by the time join() receives a TimeoutSeconds parameter, it's guaranteed not to be NaN. The current implementation is correct as-is.

🧠 Learnings used 
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7181
File: crates/stdlib/src/csv.rs:20-20
Timestamp: 2026-02-17T07:54:36.200Z
Learning: In RustPython, ensure that modules using Python sequence-like types bring in extension methods by importing the trait (for example, 'use rustpython_vm::sliceable::SliceableSequenceOp;'). Even if the trait name isn't directly referenced in code, its methods (such as len()) become in-scope on types like PyStr. Add this import in modules that rely on those extension methods to avoid compilation errors.

_ => None,
};

Expand Down
Loading