8000 Handle negative time.sleep values by ever0de · Pull Request #5906 · RustPython/RustPython · GitHub
[go: up one dir, main page]
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+44 −22
Open

Handle negative time.sleep values #5906

Show file tree
Hide file tree
Changes from all commits
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
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
50 changes: 31 additions & 19 deletions vm/src/stdlib/time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ unsafe extern "C" {
#[pymodule(name = "time", with(platform))]
mod decl {
use crate::{
PyObjectRef, PyResult, TryFromObject, VirtualMachine,
AsObject, PyObjectRef, PyResult, TryFromObject, VirtualMachine,
builtins::{PyStrRef, PyTypeRef},
function::{Either, FuncArgs, OptionalArg},
types::PyStructSequence,
Expand Down Expand Up @@ -88,10 +88,37 @@ mod decl {
duration_since_system_now(vm)
}

#[cfg(not(unix))]
#[pyfunction]
fn sleep(dur: Duration) {
std::thread::sleep(dur);
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) {
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
Member

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
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?


#[cfg(unix)]
{
// this is basically std::thread::sleep, but that catches interrupts and we don't want to;
let ts = nix::sys::time::TimeSpec::from(dur);
let res = unsafe { libc::nanosleep(ts.as_ref(), std::ptr::null_mut()) };
let interrupted = res == -1 && nix::Error::last_raw() == libc::EINTR;

if interrupted {
vm.check_signals()?;
}
}

#[cfg(not(unix))]
{
std::thread::sleep(dur);
}

Ok(())
}

#[cfg(not(target_os = "wasi"))]
Expand Down Expand Up @@ -690,21 +717,6 @@ mod platform {
get_clock_time(ClockId::CLOCK_MONOTONIC, vm)
}

#[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
Member

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


let ts = TimeSpec::from(dur);
let res = unsafe { libc::nanosleep(ts.as_ref(), std::ptr::null_mut()) };
let interrupted = res == -1 && nix::Error::last_raw() == libc::EINTR;

if interrupted {
vm.check_signals()?;
}

Ok(())
}

#[cfg(not(any(
target_os = "illumos",
target_os = "netbsd",
Expand Down
0