Skip to content
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.

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

DelayQueue::try_remove #3558

Open
qm3ster opened this issue Feb 25, 2021 · 2 comments
Open

DelayQueue::try_remove #3558

qm3ster opened this issue Feb 25, 2021 · 2 comments
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-time Module: tokio/time

Comments

@qm3ster
Copy link
Contributor

qm3ster commented Feb 25, 2021

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.

@qm3ster qm3ster added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Feb 25, 2021
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-time Module: tokio/time and removed A-tokio Area: The main tokio crate labels Feb 25, 2021
@davidpdrsn
Copy link
Member

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
Copy link
Contributor Author

qm3ster commented Mar 20, 2021

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?

erwanor added a commit to erwanor/tokio that referenced this issue Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. M-time Module: tokio/time
Projects
None yet
Development

No branches or pull requests

3 participants