fix(iroh): get rid of flume channels #2536
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 24, 2024
## Description refactor(iroh-net): replace flume in iroh-net with async_channel Rationale: see #2536 This is the first in a series of PRs that will replace flume with async_channel. In this case it is very close to a drop in replacement without any need for changes. Only noteable changes: send_async becomes send send becomes send_blocking Receiver implements Stream, so no need for .into_stream() Receiver is not Unpin, so to make it unpin you need to do `Box::pin(recv)` or use `.boxed()` ## Breaking Changes None ## Notes & open questions - Anything specific from flume we were relying on here? ## Change checklist - [x] Self-review. - [x] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [x] ~~Tests if relevant.~~ - [x] ~~All breaking changes documented.~~
put in a horrible unsafe hack to figure out if 2 senders are the same channel. To be replaced by a PR on async_channel.
also add comment about drop
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2536/docs/iroh/ Last updated: 2024-08-05T12:08:31Z |
|
Now all usage of flume in the repo are gone. The only missing thing is a blocking |
|
We still have the dependency via quic-rpc, but other than that we are flume-free. So I am going to close this. |
rklaehn
added a commit
to n0-computer/iroh-blobs
that referenced
this pull request
Oct 22, 2024
## Description refactor(iroh-net): replace flume in iroh-net with async_channel Rationale: see n0-computer/iroh#2536 This is the first in a series of PRs that will replace flume with async_channel. In this case it is very close to a drop in replacement without any need for changes. Only noteable changes: send_async becomes send send becomes send_blocking Receiver implements Stream, so no need for .into_stream() Receiver is not Unpin, so to make it unpin you need to do `Box::pin(recv)` or use `.boxed()` ## Breaking Changes None ## Notes & open questions - Anything specific from flume we were relying on here? ## Change checklist - [x] Self-review. - [x] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [x] ~~Tests if relevant.~~ - [x] ~~All breaking changes documented.~~
rklaehn
added a commit
to n0-computer/iroh-blobs
that referenced
this pull request
Oct 22, 2024
## Description refactor(iroh-net): replace flume in iroh-net with async_channel Rationale: see n0-computer/iroh#2536 This is the first in a series of PRs that will replace flume with async_channel. In this case it is very close to a drop in replacement without any need for changes. Only noteable changes: send_async becomes send send becomes send_blocking Receiver implements Stream, so no need for .into_stream() Receiver is not Unpin, so to make it unpin you need to do `Box::pin(recv)` or use `.boxed()` ## Breaking Changes None ## Notes & open questions - Anything specific from flume we were relying on here? ## Change checklist - [x] Self-review. - [x] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [x] ~~Tests if relevant.~~ - [x] ~~All breaking changes documented.~~
matheus23
pushed a commit
to n0-computer/iroh-doctor
that referenced
this pull request
Oct 22, 2024
## Description refactor(iroh-net): replace flume in iroh-net with async_channel Rationale: see n0-computer/iroh#2536 This is the first in a series of PRs that will replace flume with async_channel. In this case it is very close to a drop in replacement without any need for changes. Only noteable changes: send_async becomes send send becomes send_blocking Receiver implements Stream, so no need for .into_stream() Receiver is not Unpin, so to make it unpin you need to do `Box::pin(recv)` or use `.boxed()` ## Breaking Changes None ## Notes & open questions - Anything specific from flume we were relying on here? ## Change checklist - [x] Self-review. - [x] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [x] ~~Tests if relevant.~~ - [x] ~~All breaking changes documented.~~
matheus23
pushed a commit
that referenced
this pull request
Nov 14, 2024
## Description refactor(iroh-net): replace flume in iroh-net with async_channel Rationale: see #2536 This is the first in a series of PRs that will replace flume with async_channel. In this case it is very close to a drop in replacement without any need for changes. Only noteable changes: send_async becomes send send becomes send_blocking Receiver implements Stream, so no need for .into_stream() Receiver is not Unpin, so to make it unpin you need to do `Box::pin(recv)` or use `.boxed()` ## Breaking Changes None ## Notes & open questions - Anything specific from flume we were relying on here? ## Change checklist - [x] Self-review. - [x] ~~Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.~~ - [x] ~~Tests if relevant.~~ - [x] ~~All breaking changes documented.~~
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Get rid of flume channels
I am doing this as multiple PRs
Flume channels have a bug?(*) regarding notifications when the futures returned by recv_async are cancelled. Reproducer here: https://github.com/rklaehn/flume-bug-reproducer . Note that this does not lead to loss of messages, but loss of notifications.
(it is unclear what guarantees regarding cancel safety flume gives at all... See zesterer/flume#104 )
If you have another future like a regular timer that resolves frequently, you will never see the issue. But if you have something like an actor where the flume channel is the only source of regular events, you might be "stuck" until somebody sends another message.
So in every single location where we use select! with one of the cases being recv_async, there is a small potential to lose notifications.
Given that we use select! a lot, I think we can't afford to carefully study every single case. Replacing the select! with code that merges streams also does not seem to be a viable option.
So the only remaining option is to change to an async mpmc channel that is experimentally shown to be safe against cancellation. The only one I found is async_channel. When using async_channel in the reproducer above, the bug immediately goes away.
Breaking Changes
None
Notes & open questions
Other options?
Benchmarks?
Remove the unsafe same_channel workaround or keep it for now?
Change checklist
Documentation updates following the style guide, if relevant.Tests if relevant.All breaking changes documented.