From 3fc4bc67d95d75c10bd446e60caa8d834bc92f70 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Wed, 2 Apr 2025 16:05:44 -0700 Subject: [PATCH] queue/strategy.rs: adjust when we start the v1 singleton wheel Running the lockbud deadlock detector over the code, it thought that the Once based initialization of the wheels might recursively attempt to lock an internal mutex. Seems like a false positive to me, but if it could trigger, it would only be on startup, and we've had no reports of this being a thing. Regardless, it made lockbud think really hard about it for just under 7 hours, and adjusting the logic makes lockbud run in just under a minute, so it's worth appeasing it. Adjusting the initialization/insertion logic to look more like the v2 case makes lockbud happy and continues to pass the integration test suite. ```json { "DoubleLock": { "bug_kind": "DoubleLock", "possibility": "Probably", "diagnosis": { "first_lock_type": "ParkingLotMutex(timeq::TimeQ)", "first_lock_span": "crates/kumod/src/queue/strategy.rs:204:23: 204:45 (#0)", "second_lock_type": "ParkingLotMutex(timeq::TimeQ)", "second_lock_span": "crates/kumod/src/queue/maintainer.rs:278:17: 278:26 (#0)", "callchains": [ [ [ "crates/kumod/src/queue/strategy.rs:206:25: 206:51 (#0)" ], [ "crates/kumod/src/queue/maintainer.rs:357:5: 363:7 (#0)" ], [], [ "/home/wez/.rustup/toolchains/nightly-2025-02-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/poison/once.rs:158:41: 158:60 (#0)" ], [ "crates/kumod/src/queue/maintainer.rs:358:9: 362:11 (#0)" ], [], [ "crates/kumod/src/queue/maintainer.rs:348:17: 348:36 (#0)" ], [ "/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/runtime.rs:342:13: 342:74 (#0)" ], [ "/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/runtime.rs:368:47: 368:88 (#0)" ], [], [], [ "/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.44.1/src/runtime/scheduler/current_thread/mod.rs:211:49: 211:73 (#0)" ], [ "/home/wez/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tracing-0.1.41/src/instrument.rs:321:9: 321:23 (#0)" ], [ "crates/kumod/src/queue/maintainer.rs:359:56: 359:61 (#14652)" ], [ "crates/kumod/src/queue/maintainer.rs:290:39: 290:44 (#0)" ] ], ] } } } ``` --- crates/kumod/src/queue/strategy.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/kumod/src/queue/strategy.rs b/crates/kumod/src/queue/strategy.rs index bb3785d1..262cea4d 100644 --- a/crates/kumod/src/queue/strategy.rs +++ b/crates/kumod/src/queue/strategy.rs @@ -197,13 +197,13 @@ impl QueueStructure { } } Self::SingletonTimerWheel(q) => { - // Ensure that the msg is visible in q before we add it to - // the timer wheel, as it is possible for it to tick and pop - // the message as soon as it is inserted into the wheel. - q.lock().insert(msg.clone()); - match SINGLETON_WHEEL.lock().insert(msg.weak()) { + let mut wheel = SINGLETON_WHEEL.lock(); + match wheel.insert(msg.weak()) { Ok(()) => { + q.lock().insert(msg); + drop(wheel); start_singleton_wheel_v1(); + QueueInsertResult::Inserted { // We never notify for TimerWheel because we always tick // on a regular(ish) schedule @@ -212,8 +212,6 @@ impl QueueStructure { } Err(TimerError::Expired(_weak_msg)) => { // Message is actually due immediately. - // Take it out of the local q and return it - q.lock().remove(&msg); QueueInsertResult::Full(msg) } Err(TimerError::NotFound) => unreachable!(),