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

refactor(iroh): ActiveRelayActor terminates itself #3061

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

flub
Copy link
Contributor

@flub flub commented Dec 18, 2024

Description

This moves the logic of terminating an ActiveRelayActor from the
RelayActor to the ActiveRelayActor itself. This is nice for two
reasons:

  • The interactions between ActiveRelayActor and RelayActor are even
    simpler. RelayActor logic is now easy to reason about.

  • It will allow ActiveRelayActor to manage reconnections with
    exponential backoff better. Currently this behaviour is not changed
    but the RelayActor getting involved meant the backoff behaviour was
    influcenced in two places.

Now that the ActiveRelayActor lifecyle is much more straight forward
the RelayActor's terminating is tidied up as well.

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

This moves the logic of terminating an ActiveRelayActor from the
RelayActor to the ActiveRelayActor itself.  This is nice for two
reasons:

- The interactions between ActiveRelayActor and RelayActor are even
  simpler.  RelayActor logic is now easy to reason about.

- It will allow ActiveRelayActor to manage reconnections with
  exponential backoff better.  Currently this behaviour is not changed
  but the RelayActor getting involved meant the backoff behaviour was
  influcenced in two places.

Now that the ActiveRelayActor lifecyle is much more straight forward
the RelayActor's terminating is tidied up as well.
@flub flub requested a review from a team December 18, 2024 14:32
@flub flub self-assigned this Dec 18, 2024
@flub flub added this to the v0.31.0 milestone Dec 18, 2024
Copy link

github-actions bot commented Dec 18, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3061/docs/iroh/

Last updated: 2024-12-18T16:59:22Z

Copy link

github-actions bot commented Dec 18, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 738d124

.try_send(ActiveRelayMessage::SetHomeRelay(true))
{
error!("Send to newly started active relay failed: {err:#}.");
warn!("Home relay not set");
Copy link
Contributor

@dignifiedquire dignifiedquire Dec 18, 2024

Choose a reason for hiding this comment

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

why two log messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure...

  • One is to explain what the bug is.
  • The other explains what the impact is.

I was very close to calling .expect() on this to let it panic. I think it probably is a bug if you can't send to the inbox right after creating it (and it was nicer to have this method not be async). But I went with very noisy logging instead.

@flub flub requested a review from dignifiedquire December 18, 2024 15:41
@@ -213,7 +209,7 @@ impl ActiveRelayActor {
relay_client.send(msg.node_id, msg.packet).await
};
relay_send_fut.as_mut().set_future(fut);
self.last_write = Instant::now();
inactive_timeout.as_mut().reset(Instant::now() + RELAY_INACTIVE_CLEANUP_TIME);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure, but you could use Interval and call reset() here

}

/// Returns the handle of the [`ActiveRelayActor`].
async fn active_relay_handle(&mut self, url: &RelayUrl) -> &ActiveRelayHandle {
fn active_relay_handle(&mut self, url: &RelayUrl) -> &ActiveRelayHandle {
if !self.active_relays.contains_key(url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could probably be written nicer and more efficiently using the .entry() API

_ => Some(url.clone()),
/// Cleans up [`ActiveRelayActor`]s which have stopped running.
fn reap_active_relays(&mut self) {
let mut stopped_actors = Vec::with_capacity(self.active_relays.len());
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't have any awaits in here, so I think you could just use self.active_relays.retain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants