Skip to content

feat(iroh)!: make ProtocolHandler use async functions #3320

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Frando
Copy link
Member

@Frando Frando commented May 20, 2025

Description

This changes the ProtocolHandler trait so that the functions return impl Future instead of BoxFuture, which means implementors can simply implement the trait with async fn. We don't even need a 'static bound on these futures, so you can use &self freely without cloning and moving.

Internally, we still box the futures return by accept, on_connecting and shutdown. So performance wise this change should not have any effects. But the boxing is hidden from the user and implementing the trait now feels a lot more normal and simpler.

Breaking Changes

  • iroh::protocol::ProtocolHandler methods now return impl Future instead of BoxFuture. You can simply remove the Box::pin(async move {}) from the implementations and instead implement the methods as async fn. See the updated documentation for the iroh::protocol module for an example.
  • iroh::protocol::ProtocolHandler is no longer dyn-compatible. If you need a dyn-compatible version, you need to build your own dyn-compatible wrapper trait. See the (non-public) DynProtocolHandler in iroh::protocol as an example.

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:

@Frando Frando marked this pull request as ready for review May 20, 2025 11:35
Copy link

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

Last updated: 2025-05-20T11:35:33Z

Copy link

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

@n0bot n0bot bot added this to iroh May 20, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh May 20, 2025
@Frando Frando changed the title feat(iroh): make ProtocolHandler use async functions feat(iroh)!: make ProtocolHandler use async functions May 20, 2025
@rklaehn
Copy link
Contributor

rklaehn commented May 21, 2025

Thanks for trying this. My experience explaining Pin<Box<dyn Future...>> to very well meaning, almost enthusiastic, rust newbies, was not great. I felt a bit embarrassed for rust.

So the only overhead I can see is that if you for some reason already have a Pin<Box<dyn Future ...>>, I will have to return that as a impl Future, and it will be boxed again. But given that this interface is very high level, not the hot part of a low level function, I think it does not matter at all.

So I am very much in favour of this before 1.0.

Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

This makes me happy. DynProtocolHandler is fully internal as far as I can tell, so I think this is good!

If a user ends up double-boxing their accept futures and that indeed becomes a problem for them, well at that point they're such a power-user that switching away from the Router shouldn't be a problem for them IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants