-
Notifications
You must be signed in to change notification settings - Fork 420
#4059 followups #4118
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
#4059 followups #4118
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -593,6 +593,7 @@ where | |
peer_by_channel_id: RwLock<HashMap<ChannelId, PublicKey>>, | ||
total_pending_requests: AtomicUsize, | ||
config: LSPS2ServiceConfig, | ||
persistence_in_flight: AtomicUsize, | ||
} | ||
|
||
impl<CM: Deref, K: Deref + Clone> LSPS2ServiceHandler<CM, K> | ||
|
@@ -640,6 +641,7 @@ where | |
peer_by_intercept_scid: RwLock::new(peer_by_intercept_scid), | ||
peer_by_channel_id: RwLock::new(peer_by_channel_id), | ||
total_pending_requests: AtomicUsize::new(0), | ||
persistence_in_flight: AtomicUsize::new(0), | ||
channel_manager, | ||
kv_store, | ||
config, | ||
|
@@ -1603,7 +1605,7 @@ where | |
) -> Result<(), lightning::io::Error> { | ||
let fut = { | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
let encoded = match outer_state_lock.get(&counterparty_node_id) { | ||
match outer_state_lock.get(&counterparty_node_id) { | ||
None => { | ||
// We dropped the peer state by now. | ||
return Ok(()); | ||
|
@@ -1615,18 +1617,19 @@ where | |
return Ok(()); | ||
} else { | ||
peer_state_lock.needs_persist = false; | ||
peer_state_lock.encode() | ||
let key = counterparty_node_id.to_string(); | ||
let encoded = peer_state_lock.encode(); | ||
// Begin the write with the entry lock held. This avoids racing with | ||
// potentially-in-flight `persist` calls writing state for the same peer. | ||
self.kv_store.write( | ||
LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE, | ||
LSPS2_SERVICE_PERSISTENCE_SECONDARY_NAMESPACE, | ||
&key, | ||
encoded, | ||
) | ||
} | ||
}, | ||
}; | ||
let key = counterparty_node_id.to_string(); | ||
|
||
self.kv_store.write( | ||
LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE, | ||
LSPS2_SERVICE_PERSISTENCE_SECONDARY_NAMESPACE, | ||
&key, | ||
encoded, | ||
) | ||
} | ||
}; | ||
|
||
fut.await.map_err(|e| { | ||
|
@@ -1644,38 +1647,80 @@ where | |
// introduce some batching to upper-bound the number of requests inflight at any given | ||
// time. | ||
|
||
let mut need_remove = Vec::new(); | ||
let mut need_persist = Vec::new(); | ||
if self.persistence_in_flight.fetch_add(1, Ordering::AcqRel) > 0 { | ||
// If we're not the first event processor to get here, just return early, the increment | ||
// we just did will be treated as "go around again" at the end. | ||
return Ok(()); | ||
} | ||
|
||
{ | ||
let mut outer_state_lock = self.per_peer_state.write().unwrap(); | ||
outer_state_lock.retain(|counterparty_node_id, inner_state_lock| { | ||
let mut peer_state_lock = inner_state_lock.lock().unwrap(); | ||
peer_state_lock.prune_expired_request_state(); | ||
let is_prunable = peer_state_lock.is_prunable(); | ||
if is_prunable { | ||
need_remove.push(*counterparty_node_id); | ||
} else if peer_state_lock.needs_persist { | ||
need_persist.push(*counterparty_node_id); | ||
loop { | ||
let mut need_remove = Vec::new(); | ||
let mut need_persist = Vec::new(); | ||
|
||
{ | ||
// First build a list of peers to persist and prune with the read lock. This allows | ||
// us to avoid the write lock unless we actually need to remove a node. | ||
let outer_state_lock = self.per_peer_state.read().unwrap(); | ||
for (counterparty_node_id, inner_state_lock) in outer_state_lock.iter() { | ||
let mut peer_state_lock = inner_state_lock.lock().unwrap(); | ||
peer_state_lock.prune_expired_request_state(); | ||
let is_prunable = peer_state_lock.is_prunable(); | ||
if is_prunable { | ||
need_remove.push(*counterparty_node_id); | ||
} else if peer_state_lock.needs_persist { | ||
need_persist.push(*counterparty_node_id); | ||
} | ||
} | ||
!is_prunable | ||
}); | ||
} | ||
} | ||
|
||
for counterparty_node_id in need_persist.into_iter() { | ||
debug_assert!(!need_remove.contains(&counterparty_node_id)); | ||
self.persist_peer_state(counterparty_node_id).await?; | ||
} | ||
for counterparty_node_id in need_persist.into_iter() { | ||
debug_assert!(!need_remove.contains(&counterparty_node_id)); | ||
self.persist_peer_state(counterparty_node_id).await?; | ||
} | ||
|
||
for counterparty_node_id in need_remove { | ||
let key = counterparty_node_id.to_string(); | ||
self.kv_store | ||
.remove( | ||
LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE, | ||
LSPS2_SERVICE_PERSISTENCE_SECONDARY_NAMESPACE, | ||
&key, | ||
) | ||
.await?; | ||
for counterparty_node_id in need_remove { | ||
let mut future_opt = None; | ||
{ | ||
// We need to take the `per_peer_state` write lock to remove an entry, but also | ||
// have to hold it until after the `remove` call returns (but not through | ||
// future completion) to ensure that writes for the peer's state are | ||
// well-ordered with other `persist_peer_state` calls even across the removal | ||
// itself. | ||
let mut per_peer_state = self.per_peer_state.write().unwrap(); | ||
if let Entry::Occupied(mut entry) = per_peer_state.entry(counterparty_node_id) { | ||
let state = entry.get_mut().get_mut().unwrap(); | ||
if state.is_prunable() { | ||
entry.remove(); | ||
let key = counterparty_node_id.to_string(); | ||
future_opt = Some(self.kv_store.remove( | ||
LIQUIDITY_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE, | ||
LSPS2_SERVICE_PERSISTENCE_SECONDARY_NAMESPACE, | ||
&key, | ||
)); | ||
} else { | ||
// If the peer got new state, force a re-persist of the current state. | ||
state.needs_persist = true; | ||
} | ||
} else { | ||
// This should never happen, we can only have one `persist` call | ||
// in-progress at once and map entries are only removed by it. | ||
debug_assert!(false); | ||
} | ||
} | ||
if let Some(future) = future_opt { | ||
future.await?; | ||
} else { | ||
self.persist_peer_state(counterparty_node_id).await?; | ||
} | ||
} | ||
|
||
if self.persistence_in_flight.fetch_sub(1, Ordering::AcqRel) != 1 { | ||
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. Use 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. Hmm? Not sure which race you're referring to. I copied this logic from 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. Isn't there a potential race between 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. Hmmm, I actually think its correct. If we do a |
||
// If another thread incremented the state while we were running we should go | ||
// around again, but only once. | ||
self.persistence_in_flight.store(1, Ordering::Release); | ||
continue; | ||
} | ||
break; | ||
} | ||
|
||
Ok(()) | ||
|
Uh oh!
There was an error while loading. Please reload this page.