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

Explain why borrows can't be held across yield point in async blocks/functions #78938

Closed
jyn514 opened this issue Nov 11, 2020 · 12 comments
Closed
Assignees
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Nov 11, 2020

Consider the following code:

use std::sync::Arc;
use tokio::runtime::Runtime; // 0.3.1

async fn f() {
    let room_ref = Arc::new(Vec::new());

    let gameloop_handle = Runtime::new().unwrap().spawn(async {
        game_loop(Arc::clone(&room_ref))
    });
    gameloop_handle.await;
}

fn game_loop(v: Arc<Vec<usize>>) {}

The error message has a helpful hint:

help: to force the async block to take ownership of `room_ref` (and any other referenced variables), use the `move` keyword
  |
7 |     let gameloop_handle = Runtime::new().unwrap().spawn(async move {
8 |         game_loop(Arc::clone(&room_ref))
9 |     });

But it doesn't explain very well why move is necessary:

error[E0373]: async block may outlive the current function, but it borrows `room_ref`, which is owned by the current function
 --> src/lib.rs:7:63
  |
7 |       let gameloop_handle = Runtime::new().unwrap().spawn(async {
  |  _______________________________________________________________^
8 | |         game_loop(Arc::clone(&room_ref))
  | |                               -------- `room_ref` is borrowed here
9 | |     });
  | |_____^ may outlive borrowed value `room_ref`
  |
note: function requires argument type to outlive `'static`
 --> src/lib.rs:7:57
  |
7 |       let gameloop_handle = Runtime::new().unwrap().spawn(async {
  |  _________________________________________________________^
8 | |         game_loop(Arc::clone(&room_ref))
9 | |     });
  | |_____^

In particular, 'async block may outlive the current function' makes no sense without a very good mental model of async: the future is awaited within the current function, so of course it can't outlive it. The thing to realize here is that even though it's written as one function, it's actually two: one executed before the yield point, and one after, and the stack space for the first function goes away when you call await.

It would be nice to instead say something like

note: borrows cannot be held across a yield point, because the stack space of the current function is not preserved
help: see https://rust-lang.github.io/async-book/03_async_await/01_chapter.html#awaiting-on-a-multithreaded-executor for more information
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-async-await Area: Async & Await D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Nov 11, 2020
@jyn514
Copy link
Member Author

jyn514 commented Nov 11, 2020

cc @estebank

@jyn514
Copy link
Member Author

jyn514 commented Nov 11, 2020

Hmm, reading that link, it looks like this issue is specific to multi-threaded executors and this would be fine without a 'static bound on spawn? I'm not sure how much help the compiler can be then ...

@SkiFire13
Copy link
Contributor

This isn't specific to async, it also happens with std::thread::spawn requiring the closure to be 'static even if we later call JoinHandle::join in the same function.

@tmandry tmandry added the D-confusing Diagnostics: Confusing error or lint that should be reworked. label Dec 3, 2020
@tmandry
Copy link
Member

tmandry commented Dec 3, 2020

What about something like

note: async blocks are not executed immediately and either must take a reference or ownership of outside variables they use
help: to force ...

We could substitute "closures" in the non-async case as well, if it'd be helpful.

@tmandry tmandry added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Dec 3, 2020
@tmandry
Copy link
Member

tmandry commented Dec 3, 2020

Happy to help anyone who wants to implement this.

@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Dec 3, 2020
@sledgehammervampire
Copy link
Contributor

Hi, I want to start contributing to Rust. Would this be a good first issue?

@sledgehammervampire
Copy link
Contributor

@rustbot claim

@jyn514
Copy link
Member Author

jyn514 commented Jan 1, 2021

@1000teslas that'd be great! See https://rustc-dev-guide.rust-lang.org/ for instructions on getting started, and for your change I'd start by finding where the current error is emitted (it will be somewhere in compiler/) and looking around there.

@sledgehammervampire
Copy link
Contributor

I think the current error message is emitted from report_escaping_closure_capture in compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs. However, I'm not sure what the appropriate method for adding those new help/note messages. Would the help and note messages from DiagnosticBuilder be the right ones?

@jyn514
Copy link
Member Author

jyn514 commented Jan 2, 2021

@1000teslas yes, those are right (or at least close enough that you can fix any issues during review).

@sledgehammervampire
Copy link
Contributor

sledgehammervampire commented Jan 2, 2021

I have tried implementing the error message for async block here. I am not sure what errors an async function would have. For example, I tried messing around here with borrowing past an await point in an async function, but it seems to compile.

I am not sure how to keep lines under 100 characters when mentioning urls in an error message. Also, for some reason, ./x.py fmt doesn't seem to fix up the formatting of my files.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 16, 2021
Explain why borrows can't be held across yield point in async blocks

For rust-lang#78938.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Jan 16, 2021
Explain why borrows can't be held across yield point in async blocks

For rust-lang#78938.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Jan 16, 2021
Explain why borrows can't be held across yield point in async blocks

For rust-lang#78938.
@sledgehammervampire
Copy link
Contributor

Should this issue be closed?

@jyn514 jyn514 closed this as completed Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants