Skip to content

Handle negative time.sleep values #5906

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

Merged
youknowone merged 4 commits into from
Jul 7, 2025
Merged

Handle negative time.sleep values #5906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e6e6b9
fix(time): Handle negative sleep values
ever0de Jul 6, 2025
3aa18a9
refactor: simplify error handling in time.sleep using and_then
ever0de Jul 6, 2025
72d612c
time: merge platform-specific sleep implementations
ever0de Jul 6, 2025
60163b2
remove unused imports
ever0de Jul 6, 2025
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
Next Next commit
fix(time): Handle negative sleep values
  • Loading branch information
@ever0de
ever0de committed Jul 6, 2025
commit 0e6e6b9ccb177a26d2f53c67c1afa1fb0026716b
16 changes: 13 additions & 3 deletions vm/src/convert/try_from.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::{
builtins::PyFloat,
object::{AsObject, PyObject, PyObjectRef, PyPayload, PyRef, PyResult},
};
use malachite_bigint::Sign;
use num_traits::ToPrimitive;

/// Implemented by any type that can be created from a Python object.
Expand Down Expand Up @@ -124,10 +125,19 @@ impl<'a, T: PyPayload> TryFromBorrowedObject<'a> for &'a Py<T> {
impl TryFromObject for std::time::Duration {
fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
if let Some(float) = obj.payload::<PyFloat>() {
Ok(Self::from_secs_f64(float.to_f64()))
let f = float.to_f64();
if f < 0.0 {
return Err(vm.new_value_error("negative duration"));
}
Ok(Self::from_secs_f64(f))
} else if let Some(int) = obj.try_index_opt(vm) {
let sec = int?
.as_bigint()
let int = int?;
let bigint = int.as_bigint();
if bigint.sign() == Sign::Minus {
return Err(vm.new_value_error("negative duration"));
}

let sec = bigint
.to_u64()
.ok_or_else(|| vm.new_value_error("value out of range"))?;
Ok(Self::from_secs(sec))
Expand Down
39 changes: 35 additions & 4 deletions vm/src/stdlib/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,25 @@ mod decl {

#[cfg(not(unix))]
#[pyfunction]
fn sleep(dur: Duration) {
fn sleep(seconds: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
let dur = seconds.try_into_value::<Duration>(vm).map_err(|e| {
if e.class().is(vm.ctx.exceptions.value_error) {
// Check if this is a "negative duration" error by examining the args
if let Some(args) = e.args().first() {
if let Ok(s) = args.str(vm) {
if s.as_str() == "negative duration" {
return vm.new_value_error("sleep length must be non-negative");
}
}
}
e
} else {
e
}
})?;
Comment on lines +94 to +102
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jul 6, 2025

Choose a reason for hiding this comment

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

medium

This error mapping logic is verbose and duplicated in the unix implementation of sleep (lines 712-726). Relying on string matching for error messages can be fragile. Consider extracting this logic into a helper function to avoid duplication.

        let dur = seconds.try_into_value::<Duration>(vm).map_err(|e| {
            if e.class().is(vm.ctx.exceptions.value_error) {
                if let Some(s) = e.args().first().and_then(|arg| arg.str(vm).ok()) {
                    if s.as_str() == "negative duration" {
                        return vm.new_value_error("sleep length must be non-negative");
                    }
                }
            }
            e
        })?;

Comment on lines +93 to +102
Copy link
Copy Markdown
Member

@youknowone youknowone Jul 6, 2025

Choose a reason for hiding this comment

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

Now the two sleep functions share the same error handling.
Originally, the reason they were split was because they didn’t share any logic, so keeping them separate was simpler.
But that’s no longer the case.
Let’s remove #[cfg(not(unix))] from this function and make it the sole function responsible for validating the parameters of sleep.
We should move the cfg checks to the actual implementation instead.

Copy link
Copy Markdown
Contributor Author

@ever0de ever0de Jul 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the sleep function is implemented directly in the decl module with cfg blocks for platform-specific implementations:

#[pyfunction]
fn sleep(seconds: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
    let dur = seconds.try_into_value::<Duration>(vm).map_err(..)?;

    #[cfg(unix)]
    {
        // Unix-specific nanosleep implementation
    }
    #[cfg(not(unix))]
    {
        // std::thread::sleep for other platforms
    }
}

Other functions pattern:

// decl module
#[pyfunction]
fn monotonic(vm: &VirtualMachine) -> PyResult<f64> {
    Ok(get_monotonic_time(vm)?.as_secs_f64())
}

// platform module  
pub(super) fn get_monotonic_time(vm: &VirtualMachine) -> PyResult<Duration> {
    // Platform-specific implementation
}

Should we refactor sleep to follow the platform pattern for consistency, or it's not necessary since sleep isn't reused and would only involve simple wrapping?

Copy link
Copy Markdown
Member

@youknowone youknowone Jul 7, 2025

Choose a reason for hiding this comment

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

either way okay


std::thread::sleep(dur);
Ok(())
}

#[cfg(not(target_os = "wasi"))]
Expand Down Expand Up @@ -525,7 +542,7 @@ mod platform {
use super::decl::{SEC_TO_NS, US_TO_NS};
#[cfg_attr(target_os = "macos", allow(unused_imports))]
use crate::{
PyObject, PyRef, PyResult, TryFromBorrowedObject, VirtualMachine,
AsObject, PyObject, PyObjectRef, PyRef, PyResult, TryFromBorrowedObject, VirtualMachine,
builtins::{PyNamespace, PyStrRef},
convert::IntoPyException,
};
Expand Down Expand Up @@ -691,8 +708,22 @@ mod platform {
}

#[pyfunction]
fn sleep(dur: Duration, vm: &VirtualMachine) -> PyResult<()> {
// this is basically std::thread::sleep, but that catches interrupts and we don't want to;
Copy link
Copy Markdown
Member

@youknowone youknowone Jul 6, 2025

Choose a reason for hiding this comment

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

This comment is not expected to be removed

fn sleep(seconds: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
let dur = seconds.try_into_value::<Duration>(vm).map_err(|e| {
if e.class().is(vm.ctx.exceptions.value_error) {
// Check if this is a "negative duration" error by examining the args
if let Some(args) = e.args().first() {
if let Ok(s) = args.str(vm) {
if s.as_str() == "negative duration" {
return vm.new_value_error("sleep length must be non-negative");
}
}
}
e
} else {
e
}
})?;
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jul 6, 2025

Choose a reason for hiding this comment

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

medium

This duplicates the error mapping logic in the non-unix sleep function (lines 94-108). Code duplication can lead to maintenance issues. Extract this logic into a shared private helper function used by both sleep implementations.

        let dur = seconds.try_into_value::<Duration>(vm).map_err(|e| {
            if e.class().is(vm.ctx.exceptions.value_error) {
                if let Some(s) = e.args().first().and_then(|arg| arg.str(vm).ok()) {
                    if s.as_str() == "negative duration" {
                        return vm.new_value_error("sleep length must be non-negative");
                    }
                }
            }
            e
        })?;


let ts = TimeSpec::from(dur);
let res = unsafe { libc::nanosleep(ts.as_ref(), std::ptr::null_mut()) };
Expand Down