Skip to content

Commit 4707e6f

Browse files
authored
core: fix linked list tests reusing Registrations (#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: #1008 (comment) ## 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>
1 parent 5fb114f commit 4707e6f

File tree

1 file changed

+67
-16
lines changed

1 file changed

+67
-16
lines changed

tracing-core/src/callsite.rs

Lines changed: 67 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -276,23 +276,11 @@ impl LinkedList {
276276
mod tests {
277277
use super::*;
278278

279-
#[derive(Eq, PartialEq)]
280-
struct Cs1;
281-
static CS1: Cs1 = Cs1;
282-
static REG1: Registration = Registration::new(&CS1);
279+
struct TestCallsite;
280+
static CS1: TestCallsite = TestCallsite;
281+
static CS2: TestCallsite = TestCallsite;
283282

284-
impl Callsite for Cs1 {
285-
fn set_interest(&self, _interest: Interest) {}
286-
fn metadata(&self) -> &Metadata<'_> {
287-
unimplemented!("not needed for this test")
288-
}
289-
}
290-
291-
struct Cs2;
292-
static CS2: Cs2 = Cs2;
293-
static REG2: Registration = Registration::new(&CS2);
294-
295-
impl Callsite for Cs2 {
283+
impl Callsite for TestCallsite {
296284
fn set_interest(&self, _interest: Interest) {}
297285
fn metadata(&self) -> &Metadata<'_> {
298286
unimplemented!("not needed for this test")
@@ -301,6 +289,9 @@ mod tests {
301289

302290
#[test]
303291
fn linked_list_push() {
292+
static REG1: Registration = Registration::new(&CS1);
293+
static REG2: Registration = Registration::new(&CS2);
294+
304295
let linked_list = LinkedList::new();
305296

306297
linked_list.push(&REG1);
@@ -325,9 +316,69 @@ mod tests {
325316
});
326317
}
327318

319+
#[test]
320+
fn linked_list_push_several() {
321+
static REG1: Registration = Registration::new(&CS1);
322+
static REG2: Registration = Registration::new(&CS2);
323+
static REG3: Registration = Registration::new(&CS1);
324+
static REG4: Registration = Registration::new(&CS2);
325+
326+
let linked_list = LinkedList::new();
327+
328+
fn expect<'a>(
329+
callsites: &'a mut impl Iterator<Item = &'static Registration>,
330+
) -> impl FnMut(&'static Registration) + 'a {
331+
move |reg: &'static Registration| {
332+
let ptr = callsites
333+
.next()
334+
.expect("list contained more than the expected number of registrations!");
335+
336+
assert!(
337+
ptr::eq(reg, ptr),
338+
"Registration pointers need to match ({:?} != {:?})",
339+
reg,
340+
ptr
341+
);
342+
}
343+
}
344+
345+
linked_list.push(&REG1);
346+
linked_list.push(&REG2);
347+
let regs = [&REG2, &REG1];
348+
let mut callsites = regs.iter().copied();
349+
linked_list.for_each(expect(&mut callsites));
350+
assert!(
351+
callsites.next().is_none(),
352+
"some registrations were expected but not present: {:?}",
353+
callsites
354+
);
355+
356+
linked_list.push(&REG3);
357+
let regs = [&REG3, &REG2, &REG1];
358+
let mut callsites = regs.iter().copied();
359+
linked_list.for_each(expect(&mut callsites));
360+
assert!(
361+
callsites.next().is_none(),
362+
"some registrations were expected but not present: {:?}",
363+
callsites
364+
);
365+
366+
linked_list.push(&REG4);
367+
let regs = [&REG4, &REG3, &REG2, &REG1];
368+
let mut callsites = regs.iter().copied();
369+
linked_list.for_each(expect(&mut callsites));
370+
assert!(
371+
callsites.next().is_none(),
372+
"some registrations were expected but not present: {:?}",
373+
callsites
374+
);
375+
}
376+
328377
#[test]
329378
#[should_panic]
330379
fn linked_list_repeated() {
380+
static REG1: Registration = Registration::new(&CS1);
381+
331382
let linked_list = LinkedList::new();
332383

333384
linked_list.push(&REG1);

0 commit comments

Comments
 (0)