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

join! macro to join more than two futures? #6

Open
joshtriplett opened this issue Jul 27, 2020 · 12 comments
Open

join! macro to join more than two futures? #6

joshtriplett opened this issue Jul 27, 2020 · 12 comments

Comments

@joshtriplett
Copy link
Contributor

futures-lite implements the join function to join two futures. I'd love to have the join! macro to join more than two futures, if that'd be a reasonable addition to futures-lite.

@ghost
Copy link

ghost commented Jul 27, 2020

This is easy to add, but right now I've stumbled on an open design question.

In the futures crate, join(a, b) returns a future that outputs (A::Output, B::Output).
However, join!(a, b) desugars to join(a, b).await, which means the macro evaluates to the final output.

So the macro version join!() is not just join() that accepts an arbitrary number of arguments -- it also contains the .await.

Speaking from experience, this is sometimes confusing. Reading code, it can be hard to tell whether a macro does or does not contain an await, so you have to think extra hard to remember or check every time.

I wonder if join!() macro here should omit the .await and be used as in join!(a, b).await.

@ghost
Copy link

ghost commented Jul 27, 2020

Related: rust-lang/futures-rs#2127

@zicklag
Copy link

zicklag commented Jul 27, 2020

I think that it makes sense to have to .await join!. It seems to make it easier to understand, and it isn't at all a lot of extra syntax to add one .await. The only negative I imagine is that some people are probably already used to join! returning outputs and not futures.

Still it seems worth making the it functionally similar to the join function I think.

@joshtriplett
Copy link
Contributor Author

I agree that it makes sense to not hide the await inside the macro. However, it also seems potentially confusing to have two join! macros that do subtly different things, depending on if you use futures or futures-lite. "Potentially confusing" on a scale of "this would become the most frequent issue by far when migrating from one to the other".

Perhaps it would help to use a different name for this macro in futures-lite? That gives the opportunity for someone to see documentation.

@ghost
Copy link

ghost commented Jul 27, 2020

What if we called this zip!/zip() rather than join!/join(), taking inspiration from Option::zip() and Iterator::zip()?

Look, it makes sense:

  • fn zip<A, B>(a: Option<A>, b: Option<B>) -> Option<(A, B)>
  • fn zip<A, B>(a: impl Iterator<Item = A>, b: impl Iterator<Item = B>) -> impl Iterator<Output = (A, B)>
  • fn zip<A, B>(a: impl Future<Output = A>, b: impl Future<Output = B>) -> impl Future<Output = (A, B)>

@joshtriplett
Copy link
Contributor Author

@stjepang Sold. That makes perfect sense to me.

That would allow futures to also add zip!, since it wouldn't conflict with the existing join!. And futures-lite can just only add zip!, and mention join! in the documentation for zip! so that people searching for join! as part of porting will find what to use instead.

@zicklag
Copy link

zicklag commented Jul 29, 2020

👍 for me. I think that makes a lot of sense.

@stackinspector
Copy link

Why not add a zip_all() and/or something like FuturesUnordered?

@notgull
Copy link
Member

notgull commented Nov 27, 2022

FuturesUnordered is a bit of a footgun. If you want to implement this pattern with a substantial number of futures, you should probably use an executor like async-executor instead of this combinator.

@stackinspector
Copy link

I just wanted to simply wait for several futures in parallel, without even required for the results to be returned. Then I looked at the implementation of join_all() and found that it uses FuturesUnordered internally.

@notgull
Copy link
Member

notgull commented Nov 27, 2022

The join_all function, for a large number of futures, tends to be inefficient. You'd do better to use async_executor instead. For instance:

use async_executor::LocalExecutor;

let exec = LocalExecutor::new();
let mut handles = vec![];

for future in futures_to_run() {
    handles.push(exec.spawn(future));
}

exec.run(async move {
    for handle in handles {
        handle.await;
    }
});

@brandonros
Copy link

brandonros commented Aug 25, 2024

not sure who this may help but I was able to get really far with just futures-lite block_on and then realized I needed to add async_executor (as called out thanks to @notgull) in order to do more than 2 futures in parallel at once

use std::future::Future;

use async_executor::{LocalExecutor, Task};

pub async fn join_all<F, T, E>(futures: Vec<F>) -> Result<Vec<T>, E>
where
    F: Future<Output = Result<T, E>>,
{
    let exec = LocalExecutor::new();
    
    // Spawn each future onto the executor and collect the task handles
    let handles: Vec<Task<Result<T, E>>> = futures.into_iter()
        .map(|future| exec.spawn(future))
        .collect();

    // Run the executor and join all the tasks
    exec.run(async move {
        let mut results = Vec::with_capacity(handles.len());
        for handle in handles {
            results.push(handle.await?);
        }
        Ok(results)
    }).await
}
let mut futures = vec![];
for chunk in &chunks {
    futures.push(Robinhood::get_marketdata_options(chunk));
}
let results = utilities::join_all(futures).await?;

kayabaNerve added a commit to serai-dex/serai that referenced this issue Sep 17, 2024
…sfers

The router will now match the top-level transfer so it isn't used as the
justification for the InInstruction it's handling. This allows the theoretical
case where a top-level transfer occurs (to any entity) and an internal call
performs a transfer to Serai.

Also uses a JoinSet for fetching transactions' top-level transfers in the ERC20
crate. This does add a dependency on tokio yet improves performance, and it's
scoped under serai-processor (which is always presumed to be tokio-based).
While we could instead import futures for join_all,
smol-rs/futures-lite#6 summarizes why that wouldn't
be a good idea. While we could prefer async-executor over tokio's JoinSet,
JoinSet doesn't share the same issues as FuturesUnordered. That means our
question is solely if we want the async-executor executor or the tokio
executor, when we've already established the Serai processor is always presumed
to be tokio-based.
kayabaNerve added a commit to serai-dex/serai that referenced this issue Sep 20, 2024
…sfers

The router will now match the top-level transfer so it isn't used as the
justification for the InInstruction it's handling. This allows the theoretical
case where a top-level transfer occurs (to any entity) and an internal call
performs a transfer to Serai.

Also uses a JoinSet for fetching transactions' top-level transfers in the ERC20
crate. This does add a dependency on tokio yet improves performance, and it's
scoped under serai-processor (which is always presumed to be tokio-based).
While we could instead import futures for join_all,
smol-rs/futures-lite#6 summarizes why that wouldn't
be a good idea. While we could prefer async-executor over tokio's JoinSet,
JoinSet doesn't share the same issues as FuturesUnordered. That means our
question is solely if we want the async-executor executor or the tokio
executor, when we've already established the Serai processor is always presumed
to be tokio-based.
kayabaNerve added a commit to serai-dex/serai that referenced this issue Sep 20, 2024
…sfers

The router will now match the top-level transfer so it isn't used as the
justification for the InInstruction it's handling. This allows the theoretical
case where a top-level transfer occurs (to any entity) and an internal call
performs a transfer to Serai.

Also uses a JoinSet for fetching transactions' top-level transfers in the ERC20
crate. This does add a dependency on tokio yet improves performance, and it's
scoped under serai-processor (which is always presumed to be tokio-based).
While we could instead import futures for join_all,
smol-rs/futures-lite#6 summarizes why that wouldn't
be a good idea. While we could prefer async-executor over tokio's JoinSet,
JoinSet doesn't share the same issues as FuturesUnordered. That means our
question is solely if we want the async-executor executor or the tokio
executor, when we've already established the Serai processor is always presumed
to be tokio-based.
kayabaNerve added a commit to serai-dex/serai that referenced this issue Sep 20, 2024
…sfers

The router will now match the top-level transfer so it isn't used as the
justification for the InInstruction it's handling. This allows the theoretical
case where a top-level transfer occurs (to any entity) and an internal call
performs a transfer to Serai.

Also uses a JoinSet for fetching transactions' top-level transfers in the ERC20
crate. This does add a dependency on tokio yet improves performance, and it's
scoped under serai-processor (which is always presumed to be tokio-based).
While we could instead import futures for join_all,
smol-rs/futures-lite#6 summarizes why that wouldn't
be a good idea. While we could prefer async-executor over tokio's JoinSet,
JoinSet doesn't share the same issues as FuturesUnordered. That means our
question is solely if we want the async-executor executor or the tokio
executor, when we've already established the Serai processor is always presumed
to be tokio-based.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants