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

tokio-util: time: add DelayQueue::peek #5569

Merged
merged 7 commits into from
Apr 16, 2023

Conversation

Gelbpunkt
Copy link
Contributor

@Gelbpunkt Gelbpunkt commented Mar 21, 2023

Motivation

My goal is to implement an expiring LRU cache that uses a DashMap + DelayQueue with an optional maximum capacity. On capacity overflow, I want to remove the item that would've been next to expire. I described this issue in #5279.

Solution

Adding DelayQueue::peek allows for getting the Key that will expire next. In my usecase, a DelayQueue<K> + DashMap<K, V> is used, so after removing the next expiring key from the queue, it can also be used to remove it from the map because K is now known. I made an implementation that showcases this here.

This implementation is based on pop_entry_slot and uses peek methods instead which don't touch the contents of the stack and levels.

Closes: #5279

@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-time Module: tokio/time labels Mar 21, 2023
@Darksonn
Copy link
Contributor

The test failure isn't your fault. See #5570.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2023

However, there's a test failure now, which appears to be in your code.

@Gelbpunkt
Copy link
Contributor Author

Gelbpunkt commented Apr 3, 2023

However, there's a test failure now, which appears to be in your code.

Not quite sure why, this code shouldn't behave differently on Windows. Do you have any idea why that might be? Worst case I can try increasing the durations used.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2023

The proper fix would be to use time::pause().

@Gelbpunkt
Copy link
Contributor Author

The proper fix would be to use time::pause().

I found the issue with the original test, the entry inserted at now was instantly marked expired and therefore the assumption didn't hold. Using now + ms(1) is guaranteed to be correct, even with a paused timer. Everything should be sorted now 🎉

@Darksonn
Copy link
Contributor

Darksonn commented Apr 6, 2023

So the behavior is different for timers that have already expired? Can you please elaborate on this in the documentation?

Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@Gelbpunkt
Copy link
Contributor Author

So the behavior is different for timers that have already expired? Can you please elaborate on this in the documentation?

I've updated the documentation to reflect this

@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2023

Honestly, the behavior for expired timers worries me a bit. It would be quite surprising if you don't closely read the docs. Can we not fix it?

Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@Gelbpunkt
Copy link
Contributor Author

Honestly, the behavior for expired timers worries me a bit. It would be quite surprising if you don't closely read the docs. Can we not fix it?

Sure thing! I've restored the original test and it now considers entries marked as expired first.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Please add a test that it always returns the earliest key, even when there are multiple expired keys, no matter the order they were inserted in.

Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@Gelbpunkt
Copy link
Contributor Author

Gelbpunkt commented Apr 7, 2023

Please add a test that it always returns the earliest key, even when there are multiple expired keys, no matter the order they were inserted in.

Added. nevermind, this is impossible with the current stack for expired entries. I'll clarify it in the documentation.

Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@Gelbpunkt Gelbpunkt changed the title tokio-util: time: add DelayQueue::next_expiring tokio-util: time: add DelayQueue::peek Apr 7, 2023
Signed-off-by: Jens Reidel <adrian@travitia.xyz>
@Gelbpunkt
Copy link
Contributor Author

I've renamed the method to peek as suggested on Discord and updated the documentation to mention that the order of entries is exactly as in poll_expired.

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks.

@Darksonn Darksonn merged commit effead2 into tokio-rs:master Apr 16, 2023
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 M-time Module: tokio/time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: DelayQueue::next_expiring
2 participants