Deuxfleurs/diplonat
23
4
Fork 1
Code Issues 8 Pull requests Releases Wiki Activity

Modular Diplonat (with the option to disable useless modules) #8

Closed
adrien wants to merge 12 commits from adrien/diplonat:feature/modular-config into main
pull from: adrien/diplonat:feature/modular-config
merge into: Deuxfleurs:main
Conversation 4 Commits 12 Files changed 11 +16 -36
1 changed files with 16 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit a464aabe25 - Show all commits

52
src/diplonat.rs
View file

@ -1,4 +1,7 @@
use std::pin::Pin;
use anyhow::{anyhow, Result}; use anyhow::{anyhow, Result};
use futures::future::{try_join_all, Future};
use tokio::try_join; use tokio::try_join;
use crate::{ use crate::{
@ -6,10 +9,7 @@ use crate::{
}; };
pub struct Diplonat { pub struct Diplonat {
consul: ConsulActor, actors: Vec<Pin<Box<dyn Future<Output = Result<()>>>>>,
firewall: Option<FirewallActor>,
igd: Option<IgdActor>,
} }
impl Diplonat { impl Diplonat {
@ -18,46 +18,26 @@ impl Diplonat {
println!("{:#?}", config); println!("{:#?}", config);
let consul_actor = ConsulActor::new(config.consul); let consul_actor = ConsulActor::new(config.consul);
quentin commented 2021-09-20 16:06:44 +00:00
Review
Copy link

Now that our actors are modular, I think that this function fails to convey the real meaning of what we want to do.

IMHO, I think we want to:

  1. Instantiate actors chosen by the user
  2. Run them

With the current code, I see many problems.

((1)) I have a feeling that we are duplicating logic between the "runtime config" and our actor initialization. Currently, the constructor of the actor is only:

  • some boilerplate
  • a duplicated check found in runtime

To fix this problem, I see 2 options:

  • Delete your runtime configuration file and let actors initialize themselves from their constructor
  • Remove actors' constructor and instantiate them directly from this function only if they are required

((2)) The current try_join! fails to convey the information that we run only the initialized actors. I think that, independently of the choice you made for 1, you should create a vector of initialized actors that you will populate and then run. You will replace try_join! by try_join_all

Tip: start by solving problem number 2, it will help you solve problem number 1

Now that our actors are modular, I think that this function fails to convey the real meaning of what we want to do. IMHO, I think we want to: 1. Instantiate actors chosen by the user 2. Run them --- With the current code, I see many problems. ((1)) I have a feeling that we are duplicating logic between the "runtime config" and our actor initialization. Currently, the constructor of the actor is only: - some boilerplate - a duplicated check found in runtime To fix this problem, I see 2 options: - Delete your runtime configuration file and let actors initialize themselves from their constructor - Remove actors' constructor and instantiate them directly from this function only if they are required ((2)) The current `try_join!` fails to convey the information that we run only the initialized actors. I think that, independently of the choice you made for 1, you should create a vector of initialized actors that you will populate and then run. You will replace `try_join!` by [`try_join_all`](https://docs.rs/futures/0.3.17/futures/future/fn.try_join_all.html) *Tip: start by solving problem number 2, it will help you solve problem number 1*
let mut actors = vec![Box::pin(consul_actor.listen())];
let firewall_actor = FirewallActor::new(config.firewall, &consul_actor.rx_open_ports)?; if let Some(actor) = FirewallActor::new(config.firewall, &consul_actor.rx_open_ports)? {
actors.push(Box::pin(actor.listen()));
}
if let Some(actor) = IgdActor::new(config.igd, &consul_actor.rx_open_ports).await? {
actors.push(Box::pin(actor.listen()));
}
let igd_actor = IgdActor::new(config.igd, &consul_actor.rx_open_ports).await?; if actors.len() == 1 {
if firewall_actor.is_none() && igd_actor.is_none() {
return Err(anyhow!( return Err(anyhow!(
"At least enable *one* module, otherwise it's boring!" "At least enable *one* module, otherwise it's boring!"
)) ))
} }
let ctx = Self { Ok(Self { actors })
consul: consul_actor,
firewall: firewall_actor,
igd: igd_actor,
};
return Ok(ctx)
} }
pub async fn listen(&mut self) -> Result<()> { pub async fn listen(&self) -> Result<()> {
let firewall = &mut self.firewall; try_join_all(self.actors);
let igd = &mut self.igd; Ok(())
try_join!(
self.consul.listen(),
async {
match firewall {
Some(x) => x.listen().await,
None => Ok(()),
}
},
async {
match igd {
Some(x) => x.listen().await,
None => Ok(()),
}
},
)?;
return Ok(())
} }
} }