Release completed cancellable tasks#997
Conversation
|
I've assigned @tankyleo as a reviewer! |
Avoid retaining completed Tokio task allocations for the node lifetime while preserving shutdown cancellation and restart semantics. Co-Authored-By: HAL 9000
Remove pending LSPS request state when callers time out or are cancelled so unresponsive services cannot grow request maps. Co-Authored-By: HAL 9000
Clear per-peer connection state when the leading task is cancelled so later callers can retry and existing subscribers do not hang. Co-Authored-By: HAL 9000
Restore wallet sync status and notify waiting callers when the task performing a sync is cancelled, allowing later sync attempts to run. Co-Authored-By: HAL 9000
3553a89 to
fb5fbc8
Compare
|
Rebased after #956 landed. |
elnosh
left a comment
elnosh
left a comment
There was a problem hiding this comment.
ACK fix in 08d3b5d
I'm less familiar with the code in some of the other commits, specially in liquidity. Some of the changes implementing the different Guards took me quite a bit to understand so I would've appreciated some comments but they look correct. Specifically:
- reason for
PendingRequestGuardneeding the tokens for the pending requests inliquidity/mod.rs PendingRequestinliquidityseparating the ownerSenderfrom the otherfollowersVec<oneshot::Sender<T>>. Inconnection.rsthere's just one vec for all the senders.
These make sense now but not particularly obvious so adding comments is just a suggestion, I'm fine with it as-is
| debug_assert!( | ||
| false, | ||
| "Failed to send connection result to subscribers: {:?}", | ||
| e | ||
| ); |
There was a problem hiding this comment.
similar to fb5fbc8 this debug_assert could be removed as receiver from this pending connection attempt might have been dropped.
tankyleo
left a comment
tankyleo
left a comment
There was a problem hiding this comment.
So far just looked at the first commit, will continue tomorrow
| tasks: JoinSet<()>, | ||
| tasks: TaskTracker, | ||
| cancellation_token: CancellationToken, | ||
| accepting_tasks: bool, |
There was a problem hiding this comment.
We can delete this field here, and track is_closed on TaskTracker instead
| let mut cancellable_background_tasks = | ||
| self.cancellable_background_tasks.lock().expect("lock"); | ||
| if cancellable_background_tasks.cancellation_token.is_cancelled() { | ||
| debug_assert!( |
There was a problem hiding this comment.
I believe this debug_assert is reachable here: further below we cancel the token, then drop the lock, then wait. So here the token could be canceled, but the tasks not yet empty.
| debug_assert!(tasks.len() > 0, "Expected some cancellable background_tasks"); | ||
| tasks.abort_all(); | ||
| self.block_on(async { while let Some(_) = tasks.join_next().await {} }) | ||
| self.block_on(tasks.wait()) |
There was a problem hiding this comment.
IIRC while we are waiting on tasks.wait, we could call allow_cancellable_background_task_spawns, reopen the TaskTracker, and thus tasks.wait() would never finish.
| Self { pending_connections, node_id, active: true } | ||
| } | ||
|
|
||
| fn disarm(&mut self) { |
There was a problem hiding this comment.
nit: Is this really needed ? On drop, we'd briefly lock the pending connections, see that we have no subscribers for the node_id, and do nothing.
| drop(connection_guard); | ||
|
|
||
| assert!( | ||
| pending_connections.lock().expect("lock").is_empty(), |
There was a problem hiding this comment.
nit: Should we also make sure that key-values for a different node_id are not dropped ?
| drop(request_guard); | ||
|
|
||
| assert!( | ||
| pending_requests.lock().expect("lock").is_empty(), |
There was a problem hiding this comment.
Similar here perhaps some coverage to make sure we don't drop requests with a different request key.
| cancellable_background_tasks.tasks.spawn_on(async { future.await }, runtime_handle); | ||
| let cancellation_token = cancellable_background_tasks.cancellation_token.clone(); | ||
| // Detach the handle while the tracker continues tracking the task. | ||
| let _ = cancellable_background_tasks.tasks.spawn_on( |
There was a problem hiding this comment.
Codex:
- [P2] Preserve abort-on-drop for detached cancellable tasks _ /home/ubuntu/ldk-node/src/runtime.rs:133-133
Dropping the JoinHandle here changes the old JoinSet drop behavior: TaskTracker explicitly does not abort tracked tasks when it is dropped. With an externally supplied Tokio runtime, if Node::start() returns an error after spawning cancellable tasks but before is_running is set, Drop won't call
stop(), and these detached tasks can keep running and holding node Arcs after the node is dropped instead of being aborted as before. Please retain abort-on-drop handles or cancel/abort tracked tasks from Runtime's drop path.
This might be related to the PR we've had in mind about proper clean up after fn start
| .get(&self.request_key) | ||
| .is_some_and(|request| Arc::ptr_eq(&request.token, &self.token)) | ||
| { | ||
| pending_requests.remove(&self.request_key); |
There was a problem hiding this comment.
Codex:
- [P2] Avoid panicking on late liquidity responses _ /home/ubuntu/ldk-node/src/liquidity/mod.rs:79-79
When a caller times out or is cancelled, this guard removes the request from the pending map. For LSPS1/LSPS2 a slow LSP can still deliver the matching response later; after this removal the event handlers take their existing unknown request branch, which contains debug_assert!(false), so debug/
test builds panic and the liquidity event loop dies for any response arriving after the 5s timeout. Please either keep enough tombstone state/treat abandoned IDs as expected, or remove those assertions before deleting entries this way.
| if status_lock.register_or_subscribe_pending_sync().is_some() { | ||
| debug_assert!(false, "Sync already in progress. This should never happen."); | ||
| return; |
There was a problem hiding this comment.
Codex:
- [P2] Keep the bitcoind sync loop alive _ /home/ubuntu/ldk-node/src/chain/bitcoind.rs:157-159
If a manual bitcoind Node::sync_wallets() call owns wallet_polling_status when this background task starts (for example immediately after start() before the spawned task has registered), this branch now returns from continuously_sync_wallets and permanently stops background chain polling/fee
updates for that run. The task should wait for or otherwise handle the in-progress sync instead of exiting.
4 participants
Avoid retaining completed Tokio task allocations for the node lifetime while preserving shutdown cancellation and restart semantics. This fixes a considerable memory leak as previously we'd retain the
JoinSeted task's allocations untilstop/join_next, as tokio thankfully only notes in the https://docs.rs/tokio-util/latest/tokio_util/task/task_tracker/struct.TaskTracker.html#comparison-to-joinset.Thanks to @elnosh for reporting this leak.