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.

Sign up for GitHub

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

Jump to bottom

Implement core::future::join! #91645

Merged
merged 5 commits into from
Dec 9, 2021
Merged

Implement core::future::join! #91645

merged 5 commits into from
Dec 9, 2021

Conversation

ibraheemdev
Copy link
Member

@ibraheemdev ibraheemdev commented Dec 8, 2021 •
edited
Loading

join! polls multiple futures concurrently and returns their outputs.

async fn run() {
    let (a, b) = join!(async { 0 }, async { 1 }).await;
}

cc @rust-lang/wg-async-foundations

@rust-highfive
Copy link
Collaborator

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2021
@ibraheemdev
Copy link
Member Author

r? @joshtriplett

@rust-highfive rust-highfive assigned joshtriplett and unassigned yaahc Dec 8, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ibraheemdev ibraheemdev mentioned this pull request Dec 8, 2021
Open
4 tasks
@joshtriplett
Copy link
Member

joshtriplett commented Dec 8, 2021 •
edited
Loading

I've seen multiple people observe, in various different contexts, that join!(a, b, c) should not implicitly include an .await; instead, it should return a future, such that join!(a, b, c).await is a concurrent equivalent of (a.await, b.await, c.await).

That would also make it equivalent to the join async function, which returns a future that requires awaiting. It seems confusing to have join(a, b).await but just join!(a, b, c) with no .await.

Similarly, futures::or! doesn't include an implicit .await.

@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2021
@ibraheemdev

This comment has been minimized.

Sign in to view
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 8, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member

Since this is unstable and feature-gated, I'm going to go ahead and r+ it.

@bors r+

@bors
Copy link
Contributor

bors commented Dec 9, 2021

📌 Commit 5478f43 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 9, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2021
@bors
Auto merge of rust-lang#91691 - matthiaskrgr:rollup-wfommdr, r=matthi…
e250777 
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#91042 (Use Vec extend instead of repeated pushes on several places)
 - rust-lang#91476 (Improve 'cannot contain emoji' error.)
 - rust-lang#91568 (Pretty print break and continue without redundant space)
 - rust-lang#91645 (Implement `core::future::join!`)
 - rust-lang#91666 (update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 90c3e9a into rust-lang:master Dec 9, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 9, 2021
@nrc nrc mentioned this pull request Dec 9, 2021
Open
@danielhenrymantilla danielhenrymantilla mentioned this pull request Dec 9, 2021
Merged
1 task
danielhenrymantilla added a commit to danielhenrymantilla/rust that referenced this pull request Dec 9, 2021
@danielhenrymantilla
Minor improvements to future::join!'s implementation
e936071 
This is a follow-up from rust-lang#91645, regarding [some remarks I made](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/join!/near/264293660).

Mainly:
  - it hides the recursive munching through a private `macro`, to avoid leaking such details (a corollary is getting rid of the need to use `@` to disambiguate);
  - it uses a `match` binding, _outside_ the `async move` block, to better match the semantics from function-like syntax;
  - it pre-pins the future before calling into `poll_fn`, since `poll_fn`, alone, cannot guarantee that its capture does not move;
  - it uses `.ready()?` since it's such a neat pattern;
  - it renames `Took` to `Taken` for consistency with `Done`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
@matthiaskrgr
Rollup merge of rust-lang#91721 - danielhenrymantilla:patch-1, r=josh…
ed81098 
…triplett

Minor improvements to `future::join!`'s implementation

This is a follow-up from rust-lang#91645, regarding [some remarks I made](https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/join!/near/264293660).

Mainly:
  - it hides the recursive munching through a private `macro`, to avoid leaking such details (a corollary is getting rid of the need to use ``@`` to disambiguate);
  - it uses a `match` binding, _outside_ the `async move` block, to better match the semantics from function-like syntax;
  - it pre-pins the future before calling into `poll_fn`, since `poll_fn`, alone, cannot guarantee that its capture does not move (to clarify: I believe the previous code was sound, thanks to the outer layer of `async`. But I find it clearer / more robust to refactorings this way 🙂).
  - it uses `@ibraheemdev's` very neat `.ready()?`;
  - it renames `Took` to `Taken` for consistency with `Done` (tiny nit 😄).

~~TODO~~Done:

  - [x] Add unit tests to enforce the function-like `:value` semantics are respected.

r? `@nrc`
@yoshuawuyts
Copy link
Member

yoshuawuyts commented Dec 13, 2021 •
edited
Loading

Note: the @rust-lang/wg-async-foundations working group was not notified of this change prior to merging. The link in the opening post did not resolve, and notifications were not sent.

I would've liked to have a chance to comment on this design, as I think it is part of a larger topic of async concurrency which warrants an RFC, and should not be merged into the stdlib in a piece-meal fashion.

@yaahc
Copy link
Member

yaahc commented Dec 14, 2021

I would've liked to have a chance to comment on this design, as I think it is part of a larger topic of async concurrency which warrants an RFC, and should not be merged into the stdlib in a piece-meal fashion.

I do want to note that the reason this didn't get an RFC is because the libs team changed the requirements for adding new features and when they need RFCs in an attempt to ease the contribution experience and make it easier to experiment with new APIs. Us approving a nightly only API is in no way an endorsement that we believe a change should be stabilized, and there is still plenty of time to comment on the design. While I agree that this change and the associated story likely warrant an RFC I want to be careful that we don't overcorrect and institute process changes as a result.

@yoshuawuyts
Copy link
Member

I do want to note that the reason this didn't get an RFC is because the libs team changed the requirements for adding new features and when they need RFCs in an attempt to ease the contribution experience and make it easier to experiment with new APIs.

Ah, thanks for clarifying. I was not aware the policy had changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
8 participants
@ibraheemdev @rust-highfive @rust-log-analyzer @joshtriplett @bors @yoshuawuyts @yaahc @rustbot