tokio-rs / tokio

A runtime for writing reliable asynchronous applications with Rust. Provides I/O, networking, scheduling, timers, ...
https://tokio.rs
MIT License
27.22k stars 2.5k forks source link

DelayQueue::try_remove #3558

Open qm3ster opened 3 years ago

qm3ster commented 3 years ago

Is your feature request related to a problem? Please describe. DelayQueue::remove states:

The function panics if key is not contained by the queue.

There is no corresponding Option or Result-returning method. This means that the user has to manually ensure they don't remove a key twice, or a key that has already been yielded by poll_expired.

Describe the solution you'd like Perhaps it would be okay to have a method that returns None instead of panicking, so that Keys can be passed around less carefully, but without overhead such as keeping a HashSet of currently pending Keys alongside the DelayQueue.

Describe alternatives you've considered Another option would be replacing Key with T: <some traits>, so that the user can manually avoid collisions with semantic unique timer identifiers instead of moving literally anything into the DelayQueue but having to use the opaque non-reusable Key type.

Additional context The Key type just wraps a usize, so a colliding key from another instance of DelayQueue or just an extremely old one could exist, causing a false positive removal. Perhaps the soft-failing methods were intentionally foregone to discourage membership checking.

davidpdrsn commented 3 years ago

Is there a reason this needs Slab::try_remove and couldn't be implemented as

pub fn try_remove(&mut self, key: &Key) -> Option<Expired<T>> {
    if self.slab.contains(key.index) {
        Some(self.remove(key))
    } else {
        None
    }
}
qm3ster commented 3 years ago

Separate calls to Vec::get and Vec::get_mut, each with their own pattern match? Yeah, they might optimize the same, but it's a sensible method to have.


Having taken a look at slab, I now see my "alternatives" part is unapplicable. So the real question is in the "additional context" section: should this method exist? And if so, how much extra doc comment does it need to discourage key mixing?