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

The rules for how non-Send local variables "infect" an async function, making its Future type non-Send also, are stricter than they need to be. #63768

Open
Tracked by #69663
oconnor663 opened this issue Aug 21, 2019 · 16 comments
Labels
A-async-await Area: Async & Await AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@oconnor663
Copy link
Contributor

oconnor663 commented Aug 21, 2019

Here's an example which I think should compile, but which doesn't (cargo 1.39.0-nightly 3f700ec43 2019-08-19):

#![feature(async_await)]

fn require_send<T: Send>(_: T) {}

struct NonSendStruct { _ptr: *mut () }

async fn my_future() {
    let nonsend = NonSendStruct { _ptr: &mut () };
    async {}.await;
}

fn main() {
    require_send(my_future()); // error: `*mut ()` cannot be sent between threads safely
}

The error is "*mut () cannot be sent between threads safely", which is to say my_future() is !Send. I'm surprised by that, because the nonsend variable is never used after the .await point, and it's not Drop. Some other notes:

  • Adding a drop(nonsend) call after the let nonsend = ... line doesn't help.
  • This does compile if swap the two lines in my_future. That is, if nonsend is created after the .await point, the future is still Send.

Are there any future plans to have the rustc look more closely at which locals do or do not need to be stored across an .await?

@Nemo157
Copy link
Member

Nemo157 commented Aug 21, 2019

Even if NonSendStruct doesn't have an explicit implementation of Drop it's still dropped at the end of scope so is alive across the yield point.

Having rustc try and infer shorter scopes to kill variables before yield points sounds like it might lead to surprising situations when you try and use a variable that appears live which will suddenly make your future !Send, or adding a Drop implementation to the variable or any of its fields. It would be good for it to do so as an optimization when it doesn't visibly affect the execution, but it doesn't seem like that should affect the type checking.

See also #57478 about explicitly dropping the variable before the yield point.

@Centril Centril added A-async-await Area: Async & Await T-lang Relevant to the language team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. AsyncAwait-Unclear labels Aug 21, 2019
@oconnor663
Copy link
Contributor Author

oconnor663 commented Aug 21, 2019

Thanks for clarifying for me. While I was playing with this I was thinking about the interaction between Drop and non-lexical lifetimes. Like you can have two consecutive uses of a struct that mutably borrow the same variable, so long as the first usage is "dead" before the second starts. But then that code can break if the struct or any of its members gains a Drop implementation in the future, or if you add a new usage of the first instance after the second instance has been constructed.

In my mind, borrow checking and "Send checking" (whatever that's called) could have worked similarly, using the same "liveness" concept. Though I have no idea what's practical in the compiler, and the issue you linked makes it sound like this might be impractical. But so you see what I mean about how it feels like "liveness" could apply to both?

@oconnor663
Copy link
Contributor Author

In particular, I found it surprising that "use a nested scope" works as a solution. To me, non-lexical lifetimes were sort of about how "we don't need to use nested scopes anymore," and it seems like a shame to regress on that a little.

But then again there are a bunch of complexities around NLL that I can't think of an equivalent for here. Like Vec is marked #[may_dangle] for its contents, such that it doesn't have to "keep them alive" even though it hasn't been destructed yet. I don't know what the equivalent would be for Send-ness. If a Vec<T> is "alive but never used again" across a yield point, and T isn't Send but also doesn't need Drop, do we feel like this future is morally still Send? I think it could be. But I don't know if #[may_dangle] per se can be reused to indicate that, or if the language would need some additional feature to express it.

@RustyYato
Copy link
Contributor

@oconnor663 NLL has nothing to do with drop order, so it won't help there. NLL only changed lifetime analysis, which will allow strictly more programs, but won't change the behavior of old programs.

@oconnor663
Copy link
Contributor Author

@KrishnaSannasi for sure, I was only bringing up NLL as a metaphor for what might be possible/desirable to do about the Send/Sync/Drop -iness of async functions. It seems like the reasoning we do today around "Is this borrow alive?" could be very similar to reasoning we might want to do around "Does this object cross an .await boundary and therefore 'infect' the async function?":

  • Types which don't need Drop at all could be considered not to cross .await boundaries after their last mention.
  • Types which need Drop for their fields but don't implement Drop themselves could have .await boundary crossing evaluated independently for each field. (I believe NLL does something similar for lifetimes? If the Drop field is independent of the containing struct's lifetime parameters, those lifetimes are not extended to end of scope.)
  • Container types which implement Drop with #[may_dangle] on their contents could be treated as though the contents did not cross any .await boundaries after the container's last mention, if the contents aren't Drop. (And this could interact with the second case, if only a sub-field of the contents implements Drop.)

@RustyYato
Copy link
Contributor

Ok, that looks like it could work, altough I'm not sure if this would make undesirable changes to drop order (haven't really thought about drop order too much before, so I don't know much about the details). Someone else can evaulate that.

@oconnor663
Copy link
Contributor Author

I guess I'd frame it as "looking very closely at the existing drop order and figuring out whether a !Send object (or field) really lives past an await point."

@nikomatsakis nikomatsakis added AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. and removed AsyncAwait-Unclear labels Aug 27, 2019
@nikomatsakis nikomatsakis added AsyncAwait-OnDeck and removed AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels Oct 1, 2019
@nikomatsakis nikomatsakis added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 15, 2019
@nikomatsakis
Copy link
Contributor

I'm going to remove the 'on deck' label. I think this amounts to needing more precise analysis of what is live, which is a rather complex topic. I do hope we'll address it but leaving this marked as on-deck doesn't seem useful as it's not an issue one would pick up lightly.

@khionu
Copy link
Member

khionu commented Dec 23, 2019

To add, the compiler doesn't respect calling drop before the await point.

Using a scope: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6cd230309e9226b37804354aa36db789
Using a drop: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=626e1855f403a3f37b847ea947963559

@tbraun96
Copy link

Bumping this issue. In more complex architectures wherein there are nested scopes, the usual workaround leads to unfriendly code

@oconnor663 oconnor663 changed the title An async fn which isn't Send but which should be? The rules for how non-Send local variables "infect" an async function, making its Future type non-Send also, are stricter than they need to be. Mar 29, 2021
@nikomatsakis
Copy link
Contributor

Added to #69663

@ibraheemdev
Copy link
Member

I was recently surprised by this not compiling:

fn assert_send() {
    fn is_send(f: impl Future + Send) {}
    is_send(foo()); // ERROR: `Cell<isize>` cannot be shared between threads safely, the trait `Sync` is not implemented for `Cell<isize>`
}

fn spawn_local(x: impl Future) -> impl Future<Output = ()> {
    async { todo!() }
}

async fn foo() {
    let not_send = async {
        let v = RefCell::new(1);
        let mut u = v.borrow_mut();
        async { }.await;
        *u += 1;
    };
    spawn_local(not_send).await;
}

But this working just fine:

async fn foo() {
    let handle = spawn_local(async {
        let v = RefCell::new(1);
        let mut u = v.borrow_mut();
        async { }.await;
        *u += 1;
    });
    handle.await;
}

Even more surprisingly, this doesn't work either:

async fn foo() {
    spawn_local(async {
        let v = RefCell::new(1);
        let mut u = v.borrow_mut();
        async { }.await;
        *u += 1;
    }).await;
}

@kaimast
Copy link

kaimast commented Apr 8, 2023

Any hope this will get fixed soon?

@dingxiangfei2009
Copy link
Contributor

dingxiangfei2009 commented Mar 1, 2024

As far as I know, none of the examples here are erring out. However, I suspect that when the non-Send locals have non-trivial destructors, the infection exists unless explicit drop(..)s are inserted, of course.

So... can we check this off the list this time? Or are we still missing something? 🤔

@drakedevel
Copy link

drakedevel commented Jul 12, 2024

So... can we check this off the list this time? Or are we still missing something? 🤔

Still missing something, unfortunately. As of 1.79.0 and on Nightly 2024-07-10, this compiles:

use tokio::sync::watch::Receiver;

async fn affected_fn(recv: &Receiver<()>)  {
    {
        let b1 = recv.borrow();
        let _ = &*b1;
        drop(b1);
    }
    async {}.await;
}

pub fn test_fn(cache: &Receiver<()>) {
    fn assert_send(_f: impl std::future::Future + Send) {}
    assert_send(affected_fn(cache))
}

But this variant of affected_fn does not, with the only difference being the removal of the explicit scope:

async fn affected_fn(recv: &Receiver<()>)  {
    let b1 = recv.borrow();
    let _ = &*b1;
    drop(b1);
    async {}.await;
}

Nor does this one, which gets rid of the local entirely, and provokes a bizarre "with recv.borrow() maybe used later" diagnostic:

async fn affected_fn(recv: &Receiver<()>)  {
    let _ = &*recv.borrow();
    async {}.await;
}

Changing that line to { let _ = &*recv.borrow(); } allows it to compile, as does getting rid of the let _ = (but that provokes a warning suggesting the version that doesn't compile).

@LeHoangLong
Copy link

This is very needed in case of match expression and async, especially with non async libraries like syn

Example:

let type: syn::Type = syn::parse_str("");
match type {
  Type::BareFn(val) => {
                    drop(val);
                    let inputs = val.inputs.clone();

                    let mut referencing_type_ids: Vec<UMLBaseTypeId> = vec![];
            
                      for i in 0..inputs.len() {
                          let args = inputs[i].clone();
                          let uml_base_type = Box::pin(async_function(
                              args.clone(),
                              module_name,
                              db.clone(),
                              type_maps.clone(),
                          ))
                          .await? // error here
                      }
                }
}

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 AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests