Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(iroh-blobs): use oneshot channel from oneshot crate #2624

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Aug 14, 2024

Description

In several places we need a oneshot channel to communicate between sync and async code. In particular, there is one place where we intentionally block inside async code to be able to implement a sync callback (in the docs code, will go away at some point hopefully).

So far we have used flume and now async_channel channels with capacity 1 for this, which is quite wasteful.

This replaces all oneshot use in the fs store with the oneshot channel from the oneshot crate, which is pretty minimal and does not panic when calling blocking_recv from a tokio context.

Breaking Changes

None

Notes & open questions

Note: this was part of an attempt to get rid of async_channel in iroh-blobs. We still need it for sync interactions with the blob store from inside tokio tasks, for the current docs impl. But hopefully we should be able to replace the channel for the redb actor with a mpsc channel once that is sorted out.

Change checklist

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

Copy link

github-actions bot commented Aug 14, 2024

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

Last updated: 2024-08-15T15:07:46Z

@rklaehn rklaehn marked this pull request as ready for review August 14, 2024 14:17
@rklaehn rklaehn requested a review from dignifiedquire August 14, 2024 14:17
@rklaehn rklaehn changed the base branch from flume-purge-bytes to main August 14, 2024 15:17
…ync_channel

we can't use the one from tokio bc recv_blocking panics when used in the runtime.
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

seems fine

@rklaehn rklaehn added this pull request to the merge queue Aug 16, 2024
Merged via the queue into main with commit 2e01d47 Aug 16, 2024
28 checks passed
@rklaehn rklaehn deleted the oneshot-oneshot branch August 16, 2024 09:41
matheus23 pushed a commit that referenced this pull request Nov 14, 2024
## Description

In several places we need a oneshot channel to communicate between sync
and async code. In particular, there is one place where we intentionally
block inside async code to be able to implement a sync callback (in the
docs code, will go away at some point hopefully).

So far we have used flume and now async_channel channels with capacity 1
for this, which is quite wasteful.

This replaces all oneshot use in the fs store with the oneshot channel
from the oneshot crate, which is pretty minimal and does not panic when
calling blocking_recv from a tokio context.

## Breaking Changes

None

## Notes & open questions

Note: this was part of an attempt to get rid of async_channel in
iroh-blobs. We still need it for sync interactions with the blob store
from inside tokio tasks, for the current docs impl. But hopefully we
should be able to replace the channel for the redb actor with a mpsc
channel once that is sorted out.

## 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
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants