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

Improve comment for FuturesOrdered and FuturesUnordered #2714

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

Conversation

th4s
Copy link

@th4s th4s commented Mar 3, 2023

For FuturesOrdered there is a comment that says the futures would run in parallel, which is misleading.

This PR makes clear that futures for FuturesOrdered and FuturesUnordered run to completion concurrently instead of parallel.

Make clear that futures run to completion concurrently instead of
parallel.
@th4s th4s requested a review from taiki-e as a code owner March 3, 2023 10:46
@taiki-e
Copy link
Member

taiki-e commented Mar 5, 2023

Thanks for the PR! As #2541 mentioned, it might be a good idea to mention how to run them in parallel.

Also, could you improve the comments on the APIs mentioned in #2541 as well?

@th4s
Copy link
Author

th4s commented Mar 7, 2023

What would be the correct approach to run them in parallel? I assume you have to spawn them as tasks, but this should probably be runtime-agnostic? What is the correct way to do this? Is it this one https://docs.rs/futures-util/latest/futures_util/task/trait.SpawnExt.html#method.spawn_with_handle ?

@taiki-e
Copy link
Member

taiki-e commented Mar 10, 2023

Hmm, I’m not sure if there is a good way to do that without depending on an executor/runtime.

@th4s
Copy link
Author

th4s commented Mar 13, 2023

So by "mention how to run them in parallel" you mean I should just add the comment that one needs to invoke spawn on a runtime, or what do you have in mind?

@vasilakisfil
Copy link

I think this PR is helpful. There is some vagueness in existing docs as to what these 2 types are, and I think this PR makes it better.

As #2541 mentioned, it might be a good idea to mention how to run them in parallel.

@taiki-e I am also a bit confused by your comment. In order to run them in parallel you would have to spawn tasks, and this feels a bit out of context.

@taiki-e
Copy link
Member

taiki-e commented Apr 23, 2023

I am also a bit confused by your comment. In order to run them in parallel you would have to spawn tasks, and this feels a bit out of context.

The author of #2541 seems to think that a document that states "concurrent but not parallel" should also state "how to run in parallel" and I basically agree with that.

However, if that is difficult, simply stating "concurrent but not parallel" would be fine.

@taiki-e taiki-e added the docs label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants