-
Notifications
You must be signed in to change notification settings - Fork 385
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
lightning-liquidity
: Introduce EventQueue
notifier and wake BP for message processing
#3509
base: main
Are you sure you want to change the base?
Changes from all commits
a6b2772
31734a6
3051418
e9d09f8
a0defb7
46dfa6a
0c26bb5
68b052b
031828f
a84ea06
7d79d7a
c03a23e
9161376
ec2073e
dca0461
2372144
683cb81
ea6456f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ use crate::prelude::{Vec, VecDeque}; | |
use crate::sync::{Arc, Mutex}; | ||
|
||
use core::future::Future; | ||
#[cfg(debug_assertions)] | ||
use core::sync::atomic::{AtomicU8, Ordering}; | ||
use core::task::{Poll, Waker}; | ||
|
||
/// The maximum queue size we allow before starting to drop events. | ||
|
@@ -31,37 +33,40 @@ pub(crate) struct EventQueue { | |
queue: Arc<Mutex<VecDeque<LiquidityEvent>>>, | ||
waker: Arc<Mutex<Option<Waker>>>, | ||
#[cfg(feature = "std")] | ||
condvar: crate::sync::Condvar, | ||
condvar: Arc<crate::sync::Condvar>, | ||
#[cfg(debug_assertions)] | ||
num_held_notifier_guards: Arc<AtomicU8>, | ||
} | ||
|
||
impl EventQueue { | ||
pub fn new() -> Self { | ||
let queue = Arc::new(Mutex::new(VecDeque::new())); | ||
let waker = Arc::new(Mutex::new(None)); | ||
#[cfg(feature = "std")] | ||
{ | ||
let condvar = crate::sync::Condvar::new(); | ||
Self { queue, waker, condvar } | ||
Self { | ||
queue, | ||
waker, | ||
#[cfg(feature = "std")] | ||
condvar: Arc::new(crate::sync::Condvar::new()), | ||
#[cfg(debug_assertions)] | ||
num_held_notifier_guards: Arc::new(AtomicU8::new(0)), | ||
} | ||
#[cfg(not(feature = "std"))] | ||
Self { queue, waker } | ||
} | ||
|
||
pub fn enqueue<E: Into<LiquidityEvent>>(&self, event: E) { | ||
#[cfg(debug_assertions)] | ||
{ | ||
let mut queue = self.queue.lock().unwrap(); | ||
if queue.len() < MAX_EVENT_QUEUE_SIZE { | ||
queue.push_back(event.into()); | ||
} else { | ||
return; | ||
} | ||
let num_held_notifier_guards = self.num_held_notifier_guards.load(Ordering::Relaxed); | ||
debug_assert!( | ||
num_held_notifier_guards > 0, | ||
"We should be holding at least one notifier guard whenever enqueuing new events" | ||
); | ||
} | ||
|
||
if let Some(waker) = self.waker.lock().unwrap().take() { | ||
waker.wake(); | ||
let mut queue = self.queue.lock().unwrap(); | ||
if queue.len() < MAX_EVENT_QUEUE_SIZE { | ||
queue.push_back(event.into()); | ||
} else { | ||
return; | ||
} | ||
#[cfg(feature = "std")] | ||
self.condvar.notify_one(); | ||
} | ||
|
||
pub fn next_event(&self) -> Option<LiquidityEvent> { | ||
|
@@ -100,6 +105,81 @@ impl EventQueue { | |
pub fn get_and_clear_pending_events(&self) -> Vec<LiquidityEvent> { | ||
self.queue.lock().unwrap().split_off(0).into() | ||
} | ||
|
||
// Returns an [`EventQueueNotifierGuard`] that will notify about new event when dropped. | ||
pub fn notifier(&self) -> EventQueueNotifierGuard { | ||
#[cfg(debug_assertions)] | ||
{ | ||
self.num_held_notifier_guards.fetch_add(1, Ordering::Relaxed); | ||
} | ||
EventQueueNotifierGuard { | ||
queue: Arc::clone(&self.queue), | ||
waker: Arc::clone(&self.waker), | ||
#[cfg(feature = "std")] | ||
condvar: Arc::clone(&self.condvar), | ||
#[cfg(debug_assertions)] | ||
num_held_notifier_guards: Arc::clone(&self.num_held_notifier_guards), | ||
} | ||
} | ||
} | ||
|
||
impl Drop for EventQueue { | ||
fn drop(&mut self) { | ||
#[cfg(debug_assertions)] | ||
{ | ||
let num_held_notifier_guards = self.num_held_notifier_guards.load(Ordering::Relaxed); | ||
debug_assert!( | ||
num_held_notifier_guards == 0, | ||
"We should not be holding any notifier guards when the event queue is dropped" | ||
); | ||
} | ||
} | ||
} | ||
|
||
// A guard type that will notify about new events when dropped. | ||
#[must_use] | ||
pub(crate) struct EventQueueNotifierGuard { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we be removing the manual notifying now that the guard is in place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd also be nice if we could guarantee at compile-time that the guard is being held when queueing events There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ugh, good catch. I removed it before in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good idea, now added corresponding debug assertions. |
||
queue: Arc<Mutex<VecDeque<LiquidityEvent>>>, | ||
waker: Arc<Mutex<Option<Waker>>>, | ||
#[cfg(feature = "std")] | ||
condvar: Arc<crate::sync::Condvar>, | ||
#[cfg(debug_assertions)] | ||
num_held_notifier_guards: Arc<AtomicU8>, | ||
} | ||
|
||
impl Drop for EventQueueNotifierGuard { | ||
fn drop(&mut self) { | ||
let should_notify = !self.queue.lock().unwrap().is_empty(); | ||
|
||
if should_notify { | ||
if let Some(waker) = self.waker.lock().unwrap().take() { | ||
waker.wake(); | ||
} | ||
|
||
#[cfg(feature = "std")] | ||
self.condvar.notify_one(); | ||
} | ||
|
||
#[cfg(debug_assertions)] | ||
{ | ||
let res = self.num_held_notifier_guards.fetch_update( | ||
Ordering::Relaxed, | ||
Ordering::Relaxed, | ||
|x| Some(x.saturating_sub(1)), | ||
); | ||
match res { | ||
Ok(previous_value) if previous_value == 0 => debug_assert!( | ||
false, | ||
"num_held_notifier_guards counter out-of-sync! This should never happen!" | ||
), | ||
Err(_) => debug_assert!( | ||
false, | ||
"num_held_notifier_guards counter out-of-sync! This should never happen!" | ||
), | ||
_ => {}, | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// An event which you should probably take some action in response to. | ||
|
@@ -193,6 +273,7 @@ mod tests { | |
}); | ||
|
||
for _ in 0..3 { | ||
let _guard = event_queue.notifier(); | ||
event_queue.enqueue(expected_event.clone()); | ||
} | ||
|
||
|
@@ -218,13 +299,15 @@ mod tests { | |
let mut delayed_enqueue = false; | ||
|
||
for _ in 0..25 { | ||
let _guard = event_queue.notifier(); | ||
event_queue.enqueue(expected_event.clone()); | ||
enqueued_events.fetch_add(1, Ordering::SeqCst); | ||
} | ||
|
||
loop { | ||
tokio::select! { | ||
_ = tokio::time::sleep(Duration::from_millis(10)), if !delayed_enqueue => { | ||
let _guard = event_queue.notifier(); | ||
event_queue.enqueue(expected_event.clone()); | ||
enqueued_events.fetch_add(1, Ordering::SeqCst); | ||
delayed_enqueue = true; | ||
|
@@ -233,6 +316,7 @@ mod tests { | |
assert_eq!(e, expected_event); | ||
received_events.fetch_add(1, Ordering::SeqCst); | ||
|
||
let _guard = event_queue.notifier(); | ||
event_queue.enqueue(expected_event.clone()); | ||
enqueued_events.fetch_add(1, Ordering::SeqCst); | ||
} | ||
|
@@ -265,6 +349,7 @@ mod tests { | |
std::thread::spawn(move || { | ||
// Sleep a bit before we enqueue the events everybody is waiting for. | ||
std::thread::sleep(Duration::from_millis(20)); | ||
let _guard = thread_queue.notifier(); | ||
thread_queue.enqueue(thread_event.clone()); | ||
thread_queue.enqueue(thread_event.clone()); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this should always work, it's still possible we forget to grab the notifier in a method and it goes unnoticed due to lack of test coverage. Could we make it so that the queue is not available for access without the notifier guard? It might require the use of a sealed module as I did in #3624 with
MaybeTweakedSecretKey
.