feat(iroh-net): Add a Watchable struct for use in the Endpoint API #2806
Conversation
This adds a basic Watchable struct for use on the Endpoint API.
|
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-20T11:46:19Z |
|
@flub |
|
@mepi262 the current implementation is racy and needs fixing. What makes you interested in this? |
|
The reason in which I'm interested is followings.
I wish you good health, good luck and success. |
iroh-net/src/util/watchable.rs
Outdated
| // 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. |
There was a problem hiding this comment.
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.
|
Made some changes:
For anyone who's interested in running the loom test, you get some fun output like this: |
Description
This implements a
Watchablestruct and aWatcherwhich 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
Change checklist