[r2r] use task manager to enable lightning #1513
Conversation
…pped when the coin is deactivated
…e to update the enabling progress
shamardy
changed the title
| @@ -189,6 +190,7 @@ pub async fn init_peer_manager( | |||
| // If the user wishes to preserve privacy, addresses should likely contain only Tor Onion addresses. | |||
| let listening_addr = myipaddr(ctx).await.map_to_mm(EnableLightningError::InvalidAddress)?; | |||
| // If the listening port is used start_lightning should return an error early | |||
| // Todo: when deactivating coin the address should be unbinded, listener seems to not be dropped for some reason, should also add a unit test for this after fixing it | |||
There was a problem hiding this comment.
If I'm not mistaken, it's not unbinded due to the following conditions:
disable_coindoesn't stop the spawned futures Deactivate tokens when platform coin is deactivated #1493LightningCoinis removed from theCoinsContext::coinscollection, but the inner fields likeLightningCoin::platform,LightningCoin::peer_manageretc are shared pointers that seem to be hold in spawned futures, so they never drop.
There was a problem hiding this comment.
LightningCoin::peer_manager etc are shared pointers that seem to be hold in spawned futures, so they never drop.
Thanks for the detailed explanation, for LightningCoin::peer_manager both connect_outbound and setup_inbound from LDK's lightning_net_tokio crate spawn futures that hold the peer manager's Arc https://github.com/KomodoPlatform/atomicDEX-API/blob/fe2be27f0c96ddf5d366f5e915ea5347d1e48e43/mm2src/coins/lightning/ln_p2p.rs#L52 https://github.com/KomodoPlatform/atomicDEX-API/blob/fe2be27f0c96ddf5d366f5e915ea5347d1e48e43/mm2src/coins/lightning/ln_p2p.rs#L174
Do you think passing a weak reference to PeerManager to these functions instead can solve the problem? this of course will require a PR to be opened and accepted in rust-lightning but I don't think they will object to that. The alternative is to import lightning_net_tokio crate to mm2 and do these fixes inside mm2.
There was a problem hiding this comment.
If I remember correctly, you said DM these futures are aborted, and PeerManager and other shared data are released on stop RPC call. This means that if we abort all futures correctly, you can pass Arc pointer.
But since now we don't abort the coin related futures on disable_coin, the TcpListener is not dropped.
But anyway I think it's worth to pass Weak pointer everywhere to avoid possible undesirable cases.
There was a problem hiding this comment.
If I remember correctly, you said DM these futures are aborted, and PeerManager and other shared data are released on stop RPC call.
That is correct, I was able to re-bind to the same address:port after calling stop
mm2src/coins_activation/Cargo.toml
Outdated
| lightning = "0.0.110" | ||
| lightning-background-processor = "0.0.110" | ||
| lightning-invoice = { version = "0.18.0", features = ["serde"] } |
|
@shamardy Could you please solve git conflicts and ping Sergey for the next review iteration then? |
|
@sergeyboyko0791 this is ready for next review iteration/Re-approval :) |
This PR is needed to write the next WebDEX-lightning integration step (How to enable lightning) https://github.com/KomodoPlatform/air_dex/issues/644
BackgroundProcessorback intoLightningCoinstruct since it now gets dropped when callingstopRPC as a result of this PR [r2r] Stop spawned futures #1490 (Network graph, channel manager and scorer now get persisted on stop).start_lightningfunction tocoins_activationcrate to be able to update in progress status.