Skip to content

Commit

Permalink
fix(buffer): Consistent cmp vs == for Priority (#4022)
Browse files Browse the repository at this point in the history
Derive `Priority::Eq` from `Priority::PartialOrd`.

After INC-875, the custom `Ord` implementation for `Priority` was a
suspect for occurring panics
rust-lang/rust#129561.

It turned out the panic occurred somewhere else, but making `Eq` and
`PartialOrd` consistent cannot hurt.
  • Loading branch information
jjbayer authored Sep 12, 2024
1 parent b82aa67 commit c2a8e86
Showing 1 changed file with 34 additions and 19 deletions.
53 changes: 34 additions & 19 deletions relay-server/src/services/buffer/envelope_buffer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,11 +511,10 @@ impl<K: PartialEq, V> PartialEq for QueueItem<K, V> {

impl<K: PartialEq, V> Eq for QueueItem<K, V> {}

#[derive(Debug)]
#[derive(Debug, Clone)]
struct Priority {
readiness: Readiness,
received_at: Instant,
// FIXME(jjbayer): `last_peek` is currently never updated, see https://github.com/getsentry/relay/pull/3960.
last_peek: Instant,
}

Expand All @@ -529,22 +528,6 @@ impl Priority {
}
}

impl PartialEq for Priority {
fn eq(&self, other: &Self) -> bool {
self.readiness.ready() == other.readiness.ready()
&& self.received_at == other.received_at
&& self.last_peek == other.last_peek
}
}

impl PartialOrd for Priority {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl Eq for Priority {}

impl Ord for Priority {
fn cmp(&self, other: &Self) -> Ordering {
match (self.readiness.ready(), other.readiness.ready()) {
Expand All @@ -565,7 +548,21 @@ impl Ord for Priority {
}
}

#[derive(Debug)]
impl PartialOrd for Priority {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl PartialEq for Priority {
fn eq(&self, other: &Self) -> bool {
self.cmp(other).is_eq()
}
}

impl Eq for Priority {}

#[derive(Debug, Clone, Copy)]
struct Readiness {
own_project_ready: bool,
sampling_project_ready: bool,
Expand Down Expand Up @@ -948,6 +945,24 @@ mod tests {
assert_eq!(buffer.priority_queue.len(), 2);
}

#[test]
fn test_total_order() {
let p1 = Priority {
readiness: Readiness {
own_project_ready: true,
sampling_project_ready: true,
},
received_at: Instant::now(),
last_peek: Instant::now(),
};
let mut p2 = p1.clone();
p2.last_peek += Duration::from_millis(1);

// Last peek does not matter because project is ready:
assert_eq!(p1.cmp(&p2), Ordering::Equal);
assert_eq!(p1, p2);
}

#[tokio::test]
async fn test_last_peek_internal_order() {
let mut buffer = EnvelopeBuffer::<MemoryStackProvider>::new(mock_memory_checker());
Expand Down

0 comments on commit c2a8e86

Please sign in to comment.