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

Inconsistency in Send/Sync requirements for async/await #59245

Open
Ekleog opened this issue Mar 16, 2019 · 10 comments
Open

Inconsistency in Send/Sync requirements for async/await #59245

Ekleog opened this issue Mar 16, 2019 · 10 comments

Comments

@Ekleog
Copy link

@Ekleog Ekleog commented Mar 16, 2019

The following code, when switching between lines 14 and 15 (the two versions of the for line), oscillates between compiling and not compiling because it requires Sync for the dyn Send items:

#![feature(async_await, await_macro, futures_api)]

use std::collections::VecDeque;
use std::future::Future;

pub async fn receive<HandleFn, Fut, Ret>(handle: HandleFn) -> Ret
where
    Fut: Future<Output = ()>,
    HandleFn: Fn() -> Fut,
{
    let v: VecDeque<Box<dyn Send>> = VecDeque::new();

    let l = v.len();
    for _i in 0..v.len() {
    //for _i in 0..l {
        await!(handle());
    }
    
    loop {}
}

fn do_stuff<F: Future + Send>(_f: F) {}

pub fn showcase() {
    do_stuff(async {
        match await!(receive(async move || ())) {
            true => "test",
            false => "test",
        };
    });
}

(playground)

(Note: this is a simplification of the failure that occurs on Ekleog/erlust@98c6cbc when running cargo test)

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 18, 2019

I think this is working as intended-- I'd expect that in order for the future to be Send, all references held across await! points must be Send, which means the data they reference must be Sync. In the for loop here, a reference to v is being held across the await!. cc @nikomatsakis for another unfortunate side-effect of temporary references becoming extraordinarily visible under async/await!.

@cramertj
Copy link
Member

@cramertj cramertj commented Mar 18, 2019

I guess it's also worth pointing out that what isn't "working as intended" here is that the reference in question shouldn't need to be held across the await!, since it's only being used to produce a new temporary (the range).

@Ekleog
Copy link
Author

@Ekleog Ekleog commented Mar 18, 2019

Actually, if the reference points to inside the future itself, I think that the pointed-to stuff might only need to be Send, as there would at any point in time be only a single thread referring to them. But that is another issue (or another feature request, actually -- and I'm not even sure yet it'd be sound), and here what I wanted to point out was that the temporary borrow is propagated further than would be expected.

@earthengine
Copy link

@earthengine earthengine commented Mar 18, 2019

FYI, I was posted in internals for a similar issue regarding pattern matches.

Playground

#![feature(
    await_macro,
    async_await,
    futures_api,
    optin_builtin_traits
)]
use std::future::Future;

struct Foo;
impl Foo {
    fn foo(&self) -> Option<()> { Some(()) }
}
impl !Sync for Foo{}

async fn bar() {
    let f = Foo;
    if let Some(v) = f.foo() {
        await!(async{})
    }
}

async fn buz(f: impl Future<Output=()> + Send) {
    await!(f)
}

fn main(){
    buz(bar());
}
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 16, 2019

I believe this is a duplicate of #57017.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 16, 2019

I've marked this as deferred because of how #57017 is triaged, but I tend to think we should take some action here -- at least trying to get more precise.

question: If others agree this is a dup, maybe we can close this issue? (Perhaps move the examples over to #57017)

@Ekleog
Copy link
Author

@Ekleog Ekleog commented Apr 16, 2019

I'm not sure this is the same as #57017, but it does look similar -- will let someone more knowledgeable than me about compiler internals choose whether to close this :)

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 22, 2019

It seems like #57017 is more specifically about a code pattern that leads to this happening, the underlying issue here is just that a generator holding a self-reference across a yield point requires the referenced object to be Sync when it shouldn't, it just needs to be Send as the reference will move threads along with the referenced object. #57017 shows one possible way to cause this issue, because computing the live variables across the yield point inside a match includes the matched expression when it is unnecessary.

@Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Jul 22, 2019

Minimised example (playground):

#![feature(async_await)]

async fn foo(x: Box<dyn Send>) {
    async fn bar() {}
    let x = &x;
    bar().await;
}

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

fn main() {
    assert_send(foo(Box::new(5)));
}
error[E0277]: `dyn std::marker::Send` cannot be shared between threads safely
  --> src/main.rs:12:5
   |
12 |     assert_send(foo(Box::new(5)));
   |     ^^^^^^^^^^^ `dyn std::marker::Send` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `dyn std::marker::Send`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<dyn std::marker::Send>`
   = note: required because it appears within the type `std::boxed::Box<dyn std::marker::Send>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `&std::boxed::Box<dyn std::marker::Send>`
   = note: required because it appears within the type `for<'r, 's, 't0> {std::boxed::Box<(dyn std::marker::Send + 'r)>, &'s std::boxed::Box<(dyn std::marker::Send + 't0)>, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:3:32: 7:2 x:std::boxed::Box<(dyn std::marker::Send + 'static)> for<'r, 's, 't0> {std::boxed::Box<(dyn std::marker::Send + 'r)>, &'s std::boxed::Box<(dyn std::marker::Send + 't0)>, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:3:32: 7:2 x:std::boxed::Box<(dyn std::marker::Send + 'static)> for<'r, 's, 't0> {std::boxed::Box<(dyn std::marker::Send + 'r)>, &'s std::boxed::Box<(dyn std::marker::Send + 't0)>, impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`
note: required by `assert_send`
  --> src/main.rs:9:1
   |
9  | fn assert_send<T: Send>(_: T) {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@Ekleog
Copy link
Author

@Ekleog Ekleog commented Jun 29, 2020

I've just hit this again, with this:

use std::cell::Cell;
use std::future::Future;

#[derive(Clone)]
struct NonSend {
    foo: Cell<()>,
}

impl NonSend {
    fn get_send(&self) -> Result<&(), ()> {
        Ok(&())
    }
}

async fn bar() {}

async fn foo() -> () {
    let ns = NonSend { foo: Cell::new(()) };
    loop {
        match ns.get_send() {
            Ok(r) => return *r,
            Err(_) => bar().await,
        }
    }
}

pub fn assert_send() -> impl Send + Future<Output = ()> {
    foo()
}

(playground link)

Given that #57017 appears to be in the process of living again, maybe this should be made live again? (note: I have literally no idea how labels work for async/await)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants