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!: avoid using futures crate directly #2117

Merged
merged 35 commits into from
Apr 29, 2024
Merged

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Mar 22, 2024

Due to the low maintenance and absurd high amount of unsafe code in parts of the futures crate, we are trying to avoid usage of it.

Usages are replaced with

  • futures-lite : general Future and Stream tooling
  • futures-sink: Sink trait
  • futures-buffered: faster and safer version of Futures{Un}Ordered
  • If must be futures-util for sink specific things that are missing

Breaking Changes

  • iroh::node::Node does not implement Future anymore
  • iroh::node::Node::shutdown() is now async and can be awaited upon to wait for the node to exit
  • iroh_net::util::AbortingJoinHandles inner field is not public anymore, use the From<JoinHandle> implementation to contruct it

Followups

@Frando
Copy link
Member

Frando commented Mar 23, 2024

What about we do an iroh_base::futures module with reexports from these crates? It is rather annoying to work with (and remember!) that many crates for basic concurrency utils. And we might change crates again, and then have to go through all iroh crates again.

Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

I think I'd be -1 on iroh_base::futures.

Maybe it's worth seeing if we can get futures denied by cargo-deny?

iroh-cli/src/commands/#start.rs# Outdated Show resolved Hide resolved
@dignifiedquire dignifiedquire added this to the v0.14.0 milestone Apr 2, 2024
@flub
Copy link
Contributor

flub commented Apr 9, 2024

is this still alive? should we get it done?

@dignifiedquire
Copy link
Contributor Author

is this still alive? should we get it done?

this is blocked on n0-computer/quic-rpc#71

@dignifiedquire dignifiedquire removed this from the v0.14.0 milestone Apr 10, 2024
@rklaehn
Copy link
Contributor

rklaehn commented Apr 15, 2024

Unfortunately quic-rpc is using quite a few things from futures

  • sinks
  • streams

Changing the futures dependency in quic-rpc to futures-lite would mean that it does not play well with crates using the full futures crate anymore, which would be not very nice. Also, if we just use futures-sink instead of futures directly aren't we still depending on code from the futures project? So what do we gain?

I can switch quic-rpc to a combination of futures-lite and importing futures-sink. But I seriously don't see the benefit for all the work. I started, but it's a lot. There is no Either in futures_lite, no select, no SinkExt in futures_sink etc.

https://github.com/n0-computer/quic-rpc/tree/no-future

@dignifiedquire dignifiedquire self-assigned this Apr 22, 2024
@rklaehn
Copy link
Contributor

rklaehn commented Apr 25, 2024

@dignifiedquire I pushed a fix to your compile errors

@dignifiedquire dignifiedquire changed the title [WIP] avoid using futures crate directly refactor: avoid using futures crate directly Apr 26, 2024
@dignifiedquire dignifiedquire changed the title refactor: avoid using futures crate directly refactor!: avoid using futures crate directly Apr 26, 2024
Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

It would be nice if you could constrain futures_lite to the imports. But if you do use futures_lite::future that collides with the import of std::future that you sometimes have to do to use the anemic standardized futures api. So 🤷. If futures_lite would reexport the std futures stuff this would not be an issue, but I guess they did not want to do that.

Also I really don't get what was wrong with the BoxFuture and BoxStream type aliases in futures.

@@ -250,15 +253,6 @@ impl<D: BaoStore> Node<D> {
}
}

/// The future completes when the spawned tokio task finishes.
impl<D> Future for Node<D> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind losing this at all.

@dignifiedquire dignifiedquire added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit b91b684 Apr 29, 2024
20 of 21 checks passed
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.

4 participants