Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(netcheck): make netcheck a long-running actor #1028

Merged
merged 10 commits into from
May 19, 2023
Merged

Conversation

flub
Copy link
Contributor

@flub flub commented May 17, 2023

This turns the netcheck into a long running actor:

  • The Client creates the actor which runs in a background tokio task.
  • The Client can be cloned to get more handles to the netcheck actor.
  • Once Client is dropped the actor is stopped.
  • Now you can always pass stun packets to to netcheck: it manages the state internally.

This last point is the motivation, it simplifies how things are wired up and makes it easier to keep the right state in the netcheck actor where it belongs. This will also make future changes to netcheck more contained.

It turns the former netcheck actor into more of a full-scale actor. The ReportState is not following this pattern yet.

@flub flub marked this pull request as ready for review May 19, 2023 12:51
src/hp/netcheck.rs Outdated Show resolved Hide resolved
self.current_check_run.take();
}
ActorMessage::StunPacket { payload, from_addr } => {
// TODO: drop package if no check is running
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, first thing in the handle function does exactly that

let skip_external = self.skip_external_network;
let resolver = self.dns_resolver.clone(); // not a cheap clone, maybe Arc it
let addr = self.addr();
tokio::spawn(async move {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to track this task, and make sure it gets aborted on drop as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was wondering about that as well. But kind of wanted to defer that to a followup PR. Created #1037 for this as there's a few options to consider.

Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small nits, otherwise looks great

@flub flub merged commit 5f03510 into x-hp May 19, 2023
@flub flub deleted the flub/hp-netcheck-actor branch May 19, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants