Refactor main loop

[cyqsimon]

Main objectives

• Cleanly delimiting thread responsibilities
  • Fix the longstanding race condition at program termination; see Fix flaky tests on the ci #303
  • Separate clocks for UI refresh and data collection
    • Hopefully this will improve the process resolution success rate, reducing the number of times we have to show <UNKNOWN>; see Electron processes shown as UNKNOWN #196
• Take advantage of more modern Rust language features and APIs
• Improve readability and maintainability

Discussion

• Is this message-passing code actually better for readability and maintainability?
• Honestly, with the significantly more mature async ecosystem in 2024, should we just use tokio?

[cyqsimon]

@YJDoc2 Here's the WIP branch. It's in its initial prototype stages so please be extra critical =).

[YJDoc2]

Hey, Thanks for this. I took a quick look, and the new structure with messages seems more clean than the original impl with atomic vars.

Is this message-passing code actually better for readability and maintainability?

Atleast from first look it feels such. Will give more feedback once I'm more familiar.

Honestly, with the significantly more mature async ecosystem in 2024, should we just use tokio?

My personal opinion is async should not be added unless there is a significant benefit or requirement for it. I agree that from just message passing perspective async feels like a sensible choice, but if we could manage by native threads, personally I'd prefer that. My reasoning behind not preferring async we have to add a runtime which can be bulky, and can also run into issues with native multi threading.

That said, if you think async is the way to go, I don't have anything against it either.
I'll take a more detailed look into existing code and this implementation and come up with more suggestions. Is there anything specifically you want me to contribute to this? I can open a PR to be merged into this branch for that. Thanks for making this!

[sigmaSd]

Just saying but we have already tokio in the dependencies (for dns resolution)

cargo tree -i -p tokio
tokio v1.40.0
├── bandwhich v0.23.1 (/home/mrcool/dev/rust/bandwhich_upstream)
├── trust-dns-proto v0.23.2
│   └── trust-dns-resolver v0.23.2
│       └── bandwhich v0.23.1 (/home/mrcool/dev/rust/bandwhich_upstream)
└── trust-dns-resolver v0.23.2 (*)

[cyqsimon]

My personal opinion is async should not be added unless there is a significant benefit or requirement for it. I agree that from just message passing perspective async feels like a sensible choice, but if we could manage by native threads, personally I'd prefer that. My reasoning behind not preferring async we have to add a runtime which can be bulky, and can also run into issues with native multi threading.

Yeah I agree with basically all that's said here. Switching to async is often just substituting one source of problem with another. That being said, @sigmaSd is correct. We wouldn't be bringing in extra dependencies because it's already here.

P.s. It seems like upstream now offers a synchronous API too (although, it's still tokio behind the curtains), so we can get rid of all this:

bandwhich/src/network/dns/resolver.rs

Lines 15 to 38 in 6314f1c

	pub struct Resolver(TokioAsyncResolver);
	impl Resolver {
	pub async fn new(dns_server: Option<Ipv4Addr>) -> anyhow::Result<Self> {
	let resolver = match dns_server {
	Some(dns_server_address) => {
	let mut config = ResolverConfig::new();
	let options = ResolverOpts::default();
	let socket = SocketAddr::V4(SocketAddrV4::new(dns_server_address, 53));
	let nameserver_config = NameServerConfig {
	socket_addr: socket,
	protocol: Protocol::Udp,
	tls_dns_name: None,
	trust_negative_responses: false,
	bind_addr: None,
	};
	config.add_name_server(nameserver_config);
	TokioAsyncResolver::tokio(config, options)
	}
	None => TokioAsyncResolver::tokio_from_system_conf()?,
	};
	Ok(Self(resolver))
	}
	}