ref(iroh): Relay map extend #3577

matthiasbeyer · 2025-10-25T07:51:27Z

Description

Following #3575 (which was inspired by me complaining on your discord that this was missing from the interface) and the discussion on your discord, I figured I would fancy a slightly different approach to that PR and the interfaces introduced in your PR there.

This is twofold:

Replace the RelayMap::extend with an actual impl Extend ror RelayMap (that can easily be used with an existing RelayMap by calling rm.relays::<Vec<_>>() on it
Rewrite the existing RelayMap::extend with an RelayMap::join. Reason here is that my usecase (that, again, inspired this to be added) is to build a RelayMap object with defaults + custom entries before using them, so there's no need to update (.extend()ing) an existing RelayMap during runtime. This is still preserved via the impl Extend though.

Tell me what you think! CCing people from discord:

CC @flub
CC @dignifiedquire

Breaking Changes

RelayMap::extend (unreleased) -> RelayMap::join

flub

Thanks for writing the example in the extend docs! It's really nice to have a trait impl documented, should be done more often! Also really helpful here because of the awkward ::Vec<_> bit.

flub · 2025-10-25T08:06:46Z

iroh-relay/src/relay_map.rs

-    /// Extends this `RelayMap` with another one.
-    pub fn extend(&self, other: &RelayMap) {
+    /// Joins this `RelayMap` with another one into a new one
+    pub fn join(self, other: RelayMap) -> RelayMap {


Isn't this called chain usually? I think this is very similar to the Iterator::chain signature.

Yes it is. I opted for join because a Map is not something that can (semantically) be put into a ⛓️ chain! 😉 I think of chain() something that can be applied to a sequence of something (hence Iterator::chain but not BTreeMap::chain for example).

I have no strong feelings about the actual method name tho, so if you want me to change it, I'll happily do so.

I am not convinced passing owned versions makes sense for a fully clonable thing. This API really doesn't communicate the fact that this doesn't actually create a new version, but rather modifies the underlying, likely shared instances

flub · 2025-10-25T08:24:13Z

iroh-relay/src/relay_map.rs

-    pub fn extend(&self, other: &RelayMap) {
+    /// Joins this `RelayMap` with another one into a new one
+    pub fn join(self, other: RelayMap) -> RelayMap {
+        // SAFETY: We have exclusive access to this object


I appreciate documenting this, though I think this is slightly incorrect:

SAFETY is used to document unsafe code, panicking is not unsafe (tokio is full of panicking functions!)

This is not about exclusive access, the assertion here is that the lock is not poisoned. That is, there was no panic while holding the lock. The reasoning is that if there was a panic while holding the lock the internal invariants that the lock protects may not be upheld.

But writing that reasoning out, you can probably avoid panicking here I think. Because there are very simple internal variants you can probably just extract the internal state from the poisoned lock and put it back in there. It does require you to audit all the write paths to make sure they don't contain panicking code themselves though, because otherwise you silently lose some state. But all the write paths are internal via the functions so that's possible. It's a bunch of work for marginal gain 😄 but you can get the ideal of no panics. (Which has some small benefits to generated code size IIRC, but we've never really started optimising for this yet).

Anyway, the panics on poison are perfectly fine as well :)

SAFETY is used to document unsafe code, panicking is not unsafe (tokio is full of panicking functions!)

True. Maybe // This is safe because... rather?

This is not about exclusive access, the assertion here is that the lock is not poisoned. [...]

Also true. I need to change that.

About your last bit: Yes, but that's probably not worth the effort.

matthiasbeyer · 2025-10-25T11:31:43Z

Added some fixups for the misleading comment.

ramfox · 2025-10-30T20:14:21Z

@matthiasbeyer we are doing a release on monday! Would love to get this in if you could fix the failing CI.

Signed-off-by: Matthias Beyer <[email protected]>

matthiasbeyer · 2025-10-31T20:01:21Z

This should suffice.

I can fix issues until tomorrow, Saturday, around 13:00 UTC. After that I am enjoying Hans Zimmer and on Sunday I will be traveling 😉 👍

dignifiedquire · 2025-11-01T11:05:04Z

iroh-relay/src/relay_map.rs

+    /// # let relay_map_a: RelayMap = { unimplemented!() };
+    /// # let relay_map_b: RelayMap = { unimplemented!() };
+    ///
+    /// relay_map_a.extend(relay_map_b.relays::<Vec<_>>());


this is less efficient, and seems rather pointless to support, if you have to collect & allocate into a vector first before using the trait

The inner lock seems to stop you from doing anything nice using IntoIterator or Extend. 😞

Yep, that's the issue yes.

If this would be a DashMap, I guess it would work.

That also would mean that not the whole map must be locked when adding/removing.

But it introduces other things that are dangerous, such as holding dashmap locks over await points (which can be linted against though).

How do you lint against that? I didn't know that was possible.

Like that: https://git.hemera.systems/Hemera/dylint-various/src/branch/main/lints/dashmap-ref

😉

But clippy can do that (by now) via the await-holding-invalid-types lint: https://rust-lang.github.io/rust-clippy/stable/index.html#await_holding_invalid_type

flub

I think the main objection to the current API is that extend overlaps with Extend::extend? But implementing the trait is really ugly due to the internal locks.

The existing API does guide you to do the right thing, which really is what matters. I'm not sure supporting the Extend trait directly does improve ergonomics. So overall I think I'm -1 on this so far.

flub · 2025-11-01T11:20:22Z

iroh-relay/src/relay_map.rs

+    /// # let relay_map_a: RelayMap = { unimplemented!() };
+    /// # let relay_map_b: RelayMap = { unimplemented!() };
+    ///
+    /// relay_map_a.extend(relay_map_b.relays::<Vec<_>>());


The inner lock seems to stop you from doing anything nice using IntoIterator or Extend. 😞

flub reviewed Oct 25, 2025

View reviewed changes

flub changed the title ~~Relay map extend~~ ref(iroh): Relay map extend Oct 25, 2025

n0bot bot added this to iroh Oct 30, 2025

github-project-automation bot moved this to 🏗 In progress in iroh Oct 30, 2025

matthiasbeyer added 2 commits October 31, 2025 20:55

Add impl Extend for RelayMap

a17b61e

Signed-off-by: Matthias Beyer <[email protected]>

Replace RelayMap::extend() member fn with RelayMap::join

6edd6dc

Signed-off-by: Matthias Beyer <[email protected]>

matthiasbeyer force-pushed the relay-map-extend branch from 93d4072 to 6edd6dc Compare October 31, 2025 19:58

dignifiedquire reviewed Nov 1, 2025

View reviewed changes

flub reviewed Nov 1, 2025

View reviewed changes

ref(iroh): Relay map extend #3577

Are you sure you want to change the base?

ref(iroh): Relay map extend #3577

Uh oh!

Conversation

matthiasbeyer commented Oct 25, 2025 • edited by flub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Breaking Changes

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiasbeyer commented Oct 25, 2025

Uh oh!

ramfox commented Oct 30, 2025

Uh oh!

matthiasbeyer commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matthiasbeyer commented Oct 25, 2025 •

edited by flub

Loading