From 4707e6fe40238387cb0c1e84578c8de82ca969fe Mon Sep 17 00:00:00 2001
From: Eliza Weisman <eliza@buoyant.io>
Date: Mon, 5 Oct 2020 16:17:30 -0700
Subject: [PATCH] core: fix linked list tests reusing `Registration`s (#1016)

## Motivation

Currently, the tests for the linked list implementation in
`tracing_core::callsite` declare two static `Registration`s in the test
module, which are used in multiple tests. Since these tests are all run
in one binary, in separate threads, these statics are shared across all
tests. This means that --- depending on the order in which tests are run
--- these `Registration`s may already have values for their `next`
pointers. In particular, there's a potential issue where the `for_each`
in the test `linked_list_push` can loop repeatedly, if one of the
`Registration`s already had a next pointer from a previous test. See:
https://github.com/tokio-rs/tracing/pull/1008#issuecomment-703041964

## Solution

This branch declares separate `Registration` statics for each test.
These are not shared between multiple test threads.

We still reuse the same callsite statics, since their state does not
change during the tests --- only the `Registration`s change. I also
refactored the test code a little bit to use only a single type
implementing `Callsite`, rather than two separate types, since the
callsite impl is not actually used and this makes the code somewhat more
concise.

Finally, I added a new test for pushing more than two callsites.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
---
 tracing-core/src/callsite.rs | 83 +++++++++++++++++++++++++++++-------
 1 file changed, 67 insertions(+), 16 deletions(-)

diff --git a/tracing-core/src/callsite.rs b/tracing-core/src/callsite.rs
index 77efb3fbf7..7098adefad 100644
--- a/tracing-core/src/callsite.rs
+++ b/tracing-core/src/callsite.rs
@@ -276,23 +276,11 @@ impl LinkedList {
 mod tests {
     use super::*;
 
-    #[derive(Eq, PartialEq)]
-    struct Cs1;
-    static CS1: Cs1 = Cs1;
-    static REG1: Registration = Registration::new(&CS1);
+    struct TestCallsite;
+    static CS1: TestCallsite = TestCallsite;
+    static CS2: TestCallsite = TestCallsite;
 
-    impl Callsite for Cs1 {
-        fn set_interest(&self, _interest: Interest) {}
-        fn metadata(&self) -> &Metadata<'_> {
-            unimplemented!("not needed for this test")
-        }
-    }
-
-    struct Cs2;
-    static CS2: Cs2 = Cs2;
-    static REG2: Registration = Registration::new(&CS2);
-
-    impl Callsite for Cs2 {
+    impl Callsite for TestCallsite {
         fn set_interest(&self, _interest: Interest) {}
         fn metadata(&self) -> &Metadata<'_> {
             unimplemented!("not needed for this test")
@@ -301,6 +289,9 @@ mod tests {
 
     #[test]
     fn linked_list_push() {
+        static REG1: Registration = Registration::new(&CS1);
+        static REG2: Registration = Registration::new(&CS2);
+
         let linked_list = LinkedList::new();
 
         linked_list.push(&REG1);
@@ -325,9 +316,69 @@ mod tests {
         });
     }
 
+    #[test]
+    fn linked_list_push_several() {
+        static REG1: Registration = Registration::new(&CS1);
+        static REG2: Registration = Registration::new(&CS2);
+        static REG3: Registration = Registration::new(&CS1);
+        static REG4: Registration = Registration::new(&CS2);
+
+        let linked_list = LinkedList::new();
+
+        fn expect<'a>(
+            callsites: &'a mut impl Iterator<Item = &'static Registration>,
+        ) -> impl FnMut(&'static Registration) + 'a {
+            move |reg: &'static Registration| {
+                let ptr = callsites
+                    .next()
+                    .expect("list contained more than the expected number of registrations!");
+
+                assert!(
+                    ptr::eq(reg, ptr),
+                    "Registration pointers need to match ({:?} != {:?})",
+                    reg,
+                    ptr
+                );
+            }
+        }
+
+        linked_list.push(&REG1);
+        linked_list.push(&REG2);
+        let regs = [&REG2, &REG1];
+        let mut callsites = regs.iter().copied();
+        linked_list.for_each(expect(&mut callsites));
+        assert!(
+            callsites.next().is_none(),
+            "some registrations were expected but not present: {:?}",
+            callsites
+        );
+
+        linked_list.push(&REG3);
+        let regs = [&REG3, &REG2, &REG1];
+        let mut callsites = regs.iter().copied();
+        linked_list.for_each(expect(&mut callsites));
+        assert!(
+            callsites.next().is_none(),
+            "some registrations were expected but not present: {:?}",
+            callsites
+        );
+
+        linked_list.push(&REG4);
+        let regs = [&REG4, &REG3, &REG2, &REG1];
+        let mut callsites = regs.iter().copied();
+        linked_list.for_each(expect(&mut callsites));
+        assert!(
+            callsites.next().is_none(),
+            "some registrations were expected but not present: {:?}",
+            callsites
+        );
+    }
+
     #[test]
     #[should_panic]
     fn linked_list_repeated() {
+        static REG1: Registration = Registration::new(&CS1);
+
         let linked_list = LinkedList::new();
 
         linked_list.push(&REG1);