feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806

flub · 2024-10-15T14:59:25Z

Description

This implements a Watchable struct and a Watcher which provides access to the watched value in several ways, including some streams.

Breaking Changes

Not yet, adopting it on the API will break things.

Notes & open questions

Currently I think this is racy, see the TODO items in the code.
At the moment this emits unused compiler warnings.

Change checklist

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

This adds a basic Watchable struct for use on the Endpoint API.

github-actions · 2024-10-15T15:02:00Z

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

Last updated: 2024-11-21T13:41:33Z

mepi262 · 2024-11-08T14:05:56Z

@flub
Any update on this pull request?

flub · 2024-11-08T14:20:36Z

@mepi262 the current implementation is racy and needs fixing.

What makes you interested in this?

mepi262 · 2024-11-09T04:40:56Z

The reason in which I'm interested is followings.

I'm very much looking forward to the iroh v1.0.0.
This pull request is related to the issue, which is contained milestone v1.0.0
I'm worry about whether this pull request is merged, because this pull request has not been active since last month.
- People forget memory
- Merging pull request is difficult when time has gone, because of risk of conflict.

I wish you good health, good luck and success.

matheus23 · 2024-11-20T10:30:26Z

iroh-net/src/util/watchable.rs

+        // TODO(flub): Pretty sure this can miss a write because the epoch and wakers are
+        //    separate:
+        //    - thread 1 runs poll_next
+        //    - thread 2 runs set
+        //    1. thread 1: load epoch
+        //    2. thread 2: lock value, replace value, unlock value
+        //    3. thread 2: store epoch
+        //    4. thread 2: lock wakers, drain wakers, unlock wakers
+        //    5. thread 1: lock wakers, install waker, unlock wakers
+        //
+        //    I believe the epoch and wakers need to be stored in the same RwLock.


You're completely right about the wakers possibly missing a wakeup once a value is available.
I've tested this implementation of Watchable with loom, and can confirm that it gets stuck waiting for watcher.initialized() in some cases, even though there's a watchable.set() call.
(Interestingly, reproducing it practically requires loom usage, since it's very timing-sensitive and doesn't reproduce easily with running the same test case in a tight loop a couple thousand times.)

You're incorrect about the fact that the epoch and wakers needing to be stored on the same RwLock.

Similar to this example, after writing the waker, you can re-check the epoch to see if there was a write in-between.

Adding this second check, it passes loom.

matheus23 · 2024-11-20T11:42:02Z

Made some changes:

Added loom to watchable.rs (conditionally on the iroh_loom cfg)
Added a loom-based test that was failing in exactly the way @flub predicted
Fixed the race condition
Changed wakers from using a RwLock to using a Mutex, because we were only using RwLock::write
Removed Either by making initialized simpler

For anyone who's interested in running the loom test, you get some fun output like this:

LOOM_LOG=trace RUSTFLAGS="--cfg iroh_loom" cargo test --package iroh-net --lib -- util::watchable::tests::test_initialized_always_resolves --exact --nocapture

matheus23 · 2024-11-21T13:32:13Z

Should we (well, I) maybe address #2860 (comment) in here too?

What's the relationship between Endpoint::conn_type_stream and Connection::remote_address? It seems like remote_address should be deprecated and replaced with a method that returns a ConnectionType based on the current state of the connection.

flub

Looks good! Any reason this is still draft?

flub · 2024-11-22T08:29:15Z

iroh-net/src/util/watchable.rs

+#[derive(thiserror::Error, Debug)]
+pub enum Error {
+    #[error("Watch lost connection to underlying Watchable, it was dropped")]
+    WatchableClosed,
+}


Is there any point to having this as an enum instead of struct WatchableClosed?

I assume it would be extensibility, but then it should be marked as non-exhaustive? Doing that also makes using it harder though. So my vote is to make this a struct WatchableClosed for now.

Yeah thought about that, too. Thanks for chiming in. I'll call it Disconnected similar to what it's called in the watchable crate.

flub · 2024-11-22T08:30:26Z

iroh-net/src/util/watchable.rs

+    /// Note that the `Option` is only there to allow initialization.
+    /// Once initialized the value can never be cleared again.


This comment now belongs in the State struct?

flub · 2024-11-22T08:40:38Z

iroh-net/src/util/watchable.rs

+                if let Some(value) = state.value.clone() {
+                    // Once initialized our Option is never set back to None, but nevertheless
+                    // this code is safer without relying on that invariant.
+                    return Poll::Ready((epoch, value));


What happens to our installed waker now? We might get a spurious wakeup I guess? Which is probably the easiest way to go about this rather than trying to remove our waker again?

flub · 2024-11-22T08:47:26Z

iroh-net/src/util/watchable.rs

+impl<T: Clone + Eq> Watcher<T> {
+    /// Returns the currently held value.
+    ///
+    /// Returns `None` if the value was not set yet.


Suggested change

/// Returns `None` if the value was not set yet.

/// Returns `None` if the value was not yet initialised.

You keep spelling "initialized" wrong 😜

lol, it's so hard!

flub · 2024-11-22T08:48:51Z

iroh-net/src/util/watchable.rs

+        Ok(shared.get())
+    }
+
+    /// Returns a future completing once the value is initialized.


Suggested change

/// Returns a future completing once the value is initialized.

/// Returns a future completing once the value is initialized.

///

/// The future will complete immediately if the value was already initialized.

flub · 2024-11-22T09:00:32Z

iroh-net/src/util/watchable.rs

+    /// used to operate on the most recent value.  If the stream is not yet initialized the
+    /// first item of the stream will not be readily available.
+    ///
+    /// Note however that only the last item is stored.  If the stream is not polled when an


Suggested change

/// Note however that only the last item is stored. If the stream is not polled when an

/// Note however, that only the last item is stored. If the stream is not polled when an

flub · 2024-11-22T09:01:40Z

iroh-net/src/util/watchable.rs

+        WatchNextFut { watcher: self }
+    }
+
+    /// Returns a stream which will yield an items for the most recent value.


Suggested change

/// Returns a stream which will yield an items for the most recent value.

/// Returns a stream which will yield items containing the most recent value.

flub · 2024-11-22T09:02:27Z

iroh-net/src/util/watchable.rs

+        WatchNextStream { watcher: self }
+    }
+
+    /// Returns a stream which will yield an item for changes to the watched value.


Suggested change

/// Returns a stream which will yield an item for changes to the watched value.

/// Returns a stream which will yield items for changes to the watched value.

flub · 2024-11-22T09:03:24Z

iroh-net/src/util/watchable.rs

+}
+
+#[derive(Debug)]
+#[repr(transparent)]


Why? I thought this was a thing only needed for C APIs.

flub · 2024-11-22T09:03:34Z

iroh-net/src/util/watchable.rs

+
+#[derive(Debug)]
+#[repr(transparent)]
+pub struct WatchNextFut<'a, T> {


Needs a doc comment

flub · 2024-11-22T09:07:00Z

Should we (well, I) maybe address #2860 (comment) in here too?

What's the relationship between Endpoint::conn_type_stream and Connection::remote_address? It seems like remote_address should be deprecated and replaced with a method that returns a ConnectionType based on the current state of the connection.

I think this PR just introduces the watchable, follow-up PRs can start using it.

matheus23 · 2024-11-22T13:42:08Z

Looks good! Any reason this is still draft?

I think this PR just introduces the watchable, follow-up PRs can start using it.

Ah okay. Well my plan was to learn from the refactor of making iroh-net use this new watchable.

And I'm already finding things. E.g. do we want the Watchable to have shutdown functionality.

So, I'm still learning!
In any case, I'll split up the PR into watchable introduction & the refactor anyways.

flub · 2024-11-22T14:08:21Z

Looks good! Any reason this is still draft?

I think this PR just introduces the watchable, follow-up PRs can start using it.

Ah okay. Well my plan was to learn from the refactor of making iroh-net use this new watchable.

And I'm already finding things. E.g. do we want the Watchable to have shutdown functionality.

So, I'm still learning! In any case, I'll split up the PR into watchable introduction & the refactor anyways.

Makes sense as well. No big deal if you'd like to do this all in one place.

matheus23 and others added 2 commits October 1, 2024 17:41

feat(iroh-net): Ported & modified Watcher implementation

70f61a8

feat(iroh-net): Add a Watchable struct

f946f07

This adds a basic Watchable struct for use on the Endpoint API.

flub requested a review from dignifiedquire October 15, 2024 15:01

flub requested a review from divagant-martian October 15, 2024 15:29

matheus23 mentioned this pull request Oct 25, 2024

Tracking: WebAssembly support for iroh-net #2799

Open

matheus23 reviewed Nov 20, 2024

View reviewed changes

matheus23 added 2 commits November 20, 2024 11:37

Merge branch 'main' into flub/watchable

9982640

Test with loom, fix race condition, switch to Mutex for wakers

daa3b67

Correct spelling

1141fc9

matheus23 added 2 commits November 21, 2024 14:34

Improved Watchable and Watcher impl & API

22498c2

Whoops fix test

be17d23

flub commented Nov 22, 2024

View reviewed changes

Compiles but fails tests

0e6bdf7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806

feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806

flub commented Oct 15, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

mepi262 commented Nov 8, 2024

flub commented Nov 8, 2024

mepi262 commented Nov 9, 2024

matheus23 Nov 20, 2024

matheus23 commented Nov 20, 2024

matheus23 commented Nov 21, 2024 •

edited

Loading

flub left a comment

flub Nov 22, 2024

matheus23 Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

matheus23 Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub Nov 22, 2024

flub commented Nov 22, 2024

matheus23 commented Nov 22, 2024

flub commented Nov 22, 2024

		/// Note that the `Option` is only there to allow initialization.
		/// Once initialized the value can never be cleared again.

	/// Returns `None` if the value was not set yet.
	/// Returns `None` if the value was not yet initialised.

	/// Note however that only the last item is stored. If the stream is not polled when an
	/// Note however, that only the last item is stored. If the stream is not polled when an

	/// Returns a stream which will yield an items for the most recent value.
	/// Returns a stream which will yield items containing the most recent value.

feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806

Are you sure you want to change the base?

feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806

Conversation

flub commented Oct 15, 2024

Description

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Oct 15, 2024 • edited Loading

mepi262 commented Nov 8, 2024

flub commented Nov 8, 2024

mepi262 commented Nov 9, 2024

Choose a reason for hiding this comment

matheus23 commented Nov 20, 2024

matheus23 commented Nov 21, 2024 • edited Loading

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Nov 22, 2024

matheus23 commented Nov 22, 2024

flub commented Nov 22, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

matheus23 commented Nov 21, 2024 •

edited

Loading