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-relay)!: server actor task is not a task or actor anymore #3093

Merged
merged 8 commits into from
Jan 3, 2025

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Jan 3, 2025

Description

  • Restructures the server part of the relay to not mainly run an actor, but onyl proxy between a single actor per connection.
  • removes notifying the server about preferred/home relay status, to simplify the protocol, as this was only used for metrics

Replaces #3073

Breaking Changes

  • renamed iroh_relay::server::ClientConnRateLimit to ClientRateLimit
  • removed iroh_relay::server::MaybeTlsStreamServer from the public API
  • removed iroh_relay::client::SendMessage::NotePreferred

Notes & open questions

Change checklist

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

@dignifiedquire dignifiedquire requested a review from flub January 3, 2025 11:47
Copy link

github-actions bot commented Jan 3, 2025

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

Last updated: 2025-01-03T16:44:30Z

Copy link

github-actions bot commented Jan 3, 2025

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: 01fce78

@dignifiedquire dignifiedquire force-pushed the dig/relay-server-actors-2 branch from bdfd4dd to 0a172d8 Compare January 3, 2025 12:15
@dignifiedquire dignifiedquire changed the title refactor(iroh-relay): server actor task is not a task or actor anymore refactor(iroh-relay)!: server actor task is not a task or actor anymore Jan 3, 2025
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

Some comments, but nothing blocking really.

iroh-relay/src/server/client.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/client.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/client.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/client.rs Outdated Show resolved Hide resolved
// First priority, disco packets
packet = self.disco_send_queue.recv() => {
let packet = packet.context("Server.disco_send_queue dropped")?;
self.send_disco_packet(packet).await.context("send packet")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This actor still blocks entirely while sending data to the sink. Looking at what else it does:

  • cancel_token: will be potentially delayed if a write is slow. May not be ideal if this is used to try and stop a broken client.
  • blocks receiving obviously. Not disastrous, but it could be faster.

So all in all this is probably not a serious problem, but rather a potential nice optimisation that could be done later.

iroh-relay/src/server/client.rs Show resolved Hide resolved
iroh-relay/src/server/clients.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/clients.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/clients.rs Outdated Show resolved Hide resolved
iroh-relay/src/server/clients.rs Outdated Show resolved Hide resolved
@dignifiedquire dignifiedquire force-pushed the dig/relay-server-actors-2 branch from 061422d to c6d64fd Compare January 3, 2025 13:43
@@ -181,7 +178,7 @@ pub(crate) async fn recv_client_key<S: Stream<Item = anyhow::Result<Frame>> + Un

// TODO: variable recv size: 256 * 1024
let buf = tokio::time::timeout(
Duration::from_secs(10),
std::time::Duration::from_secs(10),
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flags, the import wasn't there anymore

@flub
Copy link
Contributor

flub commented Jan 3, 2025

Maybe best to wait with merging until the base PR is merged in main so it shows up as a separate commit?

@dignifiedquire
Copy link
Contributor Author

Maybe best to wait with merging until the base PR is merged in main so it shows up as a separate commit?

😭 yes, hopefully rebase will not be too bad..

Base automatically changed from flub/relay-actor-datagram-send-queue to main January 3, 2025 16:38
@dignifiedquire dignifiedquire force-pushed the dig/relay-server-actors-2 branch from 6a5a78c to 113519b Compare January 3, 2025 16:43
@dignifiedquire dignifiedquire added this pull request to the merge queue Jan 3, 2025
Merged via the queue into main with commit f50db17 Jan 3, 2025
25 of 26 checks passed
@dignifiedquire dignifiedquire deleted the dig/relay-server-actors-2 branch January 3, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants