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 +15 -8
1 changed files with 15 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 6015d85b73 - Show all commits

23
src/diplonat.rs
View file

@ -2,14 +2,16 @@ use std::pin::Pin;
use anyhow::{anyhow, Result}; use anyhow::{anyhow, Result};
use futures::future::{try_join_all, Future}; use futures::future::{try_join_all, Future};
use tokio::try_join; // use tokio::try_join;
type ActorTask = Pin<Box<dyn Future<Output = Result<()>>>>;
use crate::{ use crate::{
config::ConfigOpts, consul_actor::ConsulActor, fw_actor::FirewallActor, igd_actor::IgdActor, config::ConfigOpts, consul_actor::ConsulActor, fw_actor::FirewallActor, igd_actor::IgdActor,
}; };
pub struct Diplonat { pub struct Diplonat {
actors: Vec<Pin<Box<dyn Future<Output = Result<()>>>>>, actors: Vec<ActorTask>,
} }
impl Diplonat { impl Diplonat {
@ -17,14 +19,19 @@ impl Diplonat {
let config = ConfigOpts::from_env()?; let config = ConfigOpts::from_env()?;
println!("{:#?}", config); println!("{:#?}", config);
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 consul_actor = ConsulActor::new(config.consul); let mut consul_actor = ConsulActor::new(config.consul);
let mut actors = vec![Box::pin(consul_actor.listen())]; let consul_rx = consul_actor.rx_open_ports.clone();
let actor_task: ActorTask = Box::pin(consul_actor.listen());
if let Some(actor) = FirewallActor::new(config.firewall, &consul_actor.rx_open_ports)? { let mut actors = vec![actor_task];
actors.push(Box::pin(actor.listen()));
if let Some(mut actor) = FirewallActor::new(config.firewall, &consul_rx)? {
let actor_task: ActorTask = Box::pin(actor.listen());
actors.push(actor_task);
} }
if let Some(actor) = IgdActor::new(config.igd, &consul_actor.rx_open_ports).await? { if let Some(mut actor) = IgdActor::new(config.igd, &consul_rx).await? {
actors.push(Box::pin(actor.listen())); let actor_task: ActorTask = Box::pin(actor.listen());
actors.push(actor_task);
} }
if actors.len() == 1 { if actors.len() == 1 {