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

fix(iroh-willow): Remove flume dependency #2674

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

matheus23
Copy link
Contributor

Description

Purging flume as a dependency from iroh-willow.

Breaking Changes

Notes & open questions

Well, there's the whole "make a last send in impl Drop" thing.

Change checklist

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

@matheus23 matheus23 self-assigned this Aug 27, 2024
Comment on lines +206 to +230
match tokio::runtime::Handle::try_current() {
Ok(runtime) => {
let (dumb, _) = tokio::sync::mpsc::channel(1);
let inbox_tx = std::mem::replace(&mut self.inbox_tx, dumb);
runtime
.spawn(async move { inbox_tx.send(Input::Shutdown { reply: None }).await });
}
Err(_) => {
self.inbox_tx
.blocking_send(Input::Shutdown { reply: None })
.ok();
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Frando obviously this is quite a workaround for the fact that blocking_send panics if there's a runtime.

We should either move this into some central utility, or just switch to async_channel instead (like most other places, tbh).
But kind of good to know that an MPSC is technically good enough for us.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I don't have a better solution OTOH. @rklaehn fought this hard as well.

Copy link

github-actions bot commented Aug 27, 2024

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

Last updated: 2024-08-28T06:45:16Z

@Frando Frando force-pushed the willow-rpc branch 3 times, most recently from 81cf06d to 74f90ad Compare August 27, 2024 16:40
@Frando
Copy link
Member

Frando commented Aug 27, 2024

Ah, sorry, I force-pushed the willow-rpc branch.
Your commits apply cleanly on top of the force-pushed changes though:
https://github.com/n0-computer/iroh/compare/willow-rpc...Frando/willow-flume-purge?expand=1
so you can just push Frando/willow-flume-purge to your branch (didn't want to mess with your branch further).

My current working with the various willow branches was:

  • No rebases/force pushes on the willow branch. Merge main into it regularily.
  • Occasionally rebase feature branches against latest willow branch

Maybe should stop the latter and instead merge willow into the feature branches, now that I'm not the only one working on it :D

@matheus23 matheus23 changed the title Matheus23/willow flume purge fix(iroh-willow): Remove flume dependency Aug 28, 2024
@matheus23 matheus23 changed the base branch from willow-rpc to willow August 28, 2024 06:44
@matheus23
Copy link
Contributor Author

Ah, sorry, I force-pushed the willow-rpc branch.

Don't worry about it! I was just lazy and based the branch on top of willow-rpc instead of willow, which would've been the sane choice.

I changed the base to willow now and cherry-picked the changes. It all applies cleanly as far as I can tell.

@matheus23 matheus23 merged commit 3484f61 into willow Aug 28, 2024
24 of 27 checks passed
@matheus23 matheus23 deleted the matheus23/willow-flume-purge branch August 28, 2024 13:59
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