Skip to content

*: make IfWatcher::new synchronous#24

Merged
mxinden merged 3 commits into
libp2p:masterfrom
elenaf9:synchronous-IfWatcher-new
Aug 10, 2022
Merged

*: make IfWatcher::new synchronous#24
mxinden merged 3 commits into
libp2p:masterfrom
elenaf9:synchronous-IfWatcher-new

Conversation

@elenaf9

@elenaf9 elenaf9 commented Aug 9, 2022

Copy link
Copy Markdown
Member

Make IfWatcher::new synchronous. The only reason why it is currently async is because the linux implementation executed a AddressGetRequest in new to obtain the initial list of ip addresses. By chaining this stream together with the existing message stream we can make linux::IfWatcher::new sync and instead move the polling into the Stream impl.
Motivation for this change is that IfWatcher::new being async currently causes some complexity in rust-libp2p. Concretely, it forces to wrap the IfWatcher in an enum [1] and first poll the future returned by IfWatcher::new [2] before we can poll the IfWatcher itself.

@mxinden mxinden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Cool. This looks good to me.

@mxinden

mxinden commented Aug 10, 2022

Copy link
Copy Markdown
Member

@elenaf9 could you resolve the merge conflicts?

@mxinden mxinden left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, missing changelog entry.

Comment thread CHANGELOG.md

### Changed
- Add `IfWatcher::poll_next`. Implement `Stream` instead of `Future` for `IfWatcher`. See [PR 23].
- Make `IfWatcher::new` synchronous. See [PR 24].

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure how to describe this better.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

Comment thread CHANGELOG.md

### Changed
- Add `IfWatcher::poll_next`. Implement `Stream` instead of `Future` for `IfWatcher`. See [PR 23].
- Make `IfWatcher::new` synchronous. See [PR 24].

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@mxinden
mxinden merged commit 137bd31 into libp2p:master Aug 10, 2022
@mxinden

mxinden commented Aug 10, 2022

Copy link
Copy Markdown
Member

@elenaf9 should I cut a release already, or do you expect other changes in the near future?

@elenaf9

elenaf9 commented Aug 10, 2022

Copy link
Copy Markdown
Member Author

@elenaf9 should I cut a release already, or do you expect other changes in the near future?

I am still looking into simplifying the IfWatcher integration in rust-libp2p. There may still be some minor changes coming. I will ping you once I am through and we can cut a release. Thanks!

@elenaf9
elenaf9 deleted the synchronous-IfWatcher-new branch August 10, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants