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

Add HashDelayQueue utility #139

Merged
merged 1 commit into from Oct 19, 2021

Conversation

jacobkaufmann
Copy link
Collaborator

@jacobkaufmann jacobkaufmann commented Oct 15, 2021

Add a HashDelayQueue struct that can act as a generator that yields "expired" items. This struct is intended for the implementation of routing table management. With a HashDelayQueue, we can insert keys with a given timeout or delay, and when polled, the delay queue will yield an expired item (if one exists).

  • Create a directory for the trin_core::utils module.
  • Create a HashDelayQueue utility in the trin_core::utils module.

@jacobkaufmann jacobkaufmann force-pushed the hash-delay-queue branch 3 times, most recently from 301651a to d22eca6 Compare October 15, 2021 16:38
Copy link
Collaborator

@mrferris mrferris left a comment

Choose a reason for hiding this comment

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

Looks good to me! I wasn't yet aware of the testing macros that tokio_test provides, those are very nice.

Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Looks great!


sleep(Duration::from_millis(1)).await;

let entry = assert_ready_ok_some!(poll!(queue));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels a little strange to me that an assert_* statement returns some kind of value... but I see that's also the behavior of some of these tokio assert_*s so it's consistent

K: std::cmp::Eq + std::hash::Hash + std::clone::Clone + Unpin,
{
/// The given entries.
entries: HashMap<K, delay_queue::Key>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe expand the comment just a bit here to explain the value

/// Resets the expiration timeout of an entry for `key`. Returns `true` if the key existed,
/// `false` otherwise.
///
/// # Panics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why panic (here and insert_with_timeout() instead of returning a Result of some kind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The panic here is because that is how the tokio-util library is handling the exception. It looks like the maximum duration is ~2 years, so I think a panic is acceptable here. We should never insert anything with such a long delay.

#[tokio::test]
async fn clear() {
time::pause();

Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick - but it seems trivial to go ahead and bump up the # of entries here and in len() to something like 3. I wouldn't call it necessary, you just never know what bugs are hiding above the 0-1 range

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good callout! I'll update the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants