iroh icon indicating copy to clipboard operation
iroh copied to clipboard

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

Open flub opened this issue 1 year ago • 1 comments

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.

flub avatar Oct 15 '24 14:10 flub

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

Last updated: 2024-12-13T09:40:10Z

github-actions[bot] avatar Oct 15 '24 15:10 github-actions[bot]

@flub Any update on this pull request?

mepi262 avatar Nov 08 '24 14:11 mepi262

@mepi262 the current implementation is racy and needs fixing.

What makes you interested in this?

flub avatar Nov 08 '24 14:11 flub

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.

mepi262 avatar Nov 09 '24 04:11 mepi262

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 avatar Nov 20 '24 11:11 matheus23

Should we (well, I) maybe address https://github.com/n0-computer/iroh/discussions/2860#discussioncomment-11239318 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.

matheus23 avatar Nov 21 '24 13:11 matheus23

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.

flub avatar Nov 22 '24 09:11 flub

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.

matheus23 avatar Nov 22 '24 13:11 matheus23

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.

flub avatar Nov 22 '24 14:11 flub

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: 5066045

github-actions[bot] avatar Dec 10 '24 10:12 github-actions[bot]