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

Missing detection of Send for async fn #53259

Closed
Ekleog opened this issue Aug 10, 2018 · 8 comments
Closed

Missing detection of Send for async fn #53259

Ekleog opened this issue Aug 10, 2018 · 8 comments
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Ekleog
Copy link

Ekleog commented Aug 10, 2018

The following async function appears to not implement Send: (simpler reproducer at second post from @Nemo157 using generators)

pub async fn __receive() -> ()
{
    let mut chan: futures::channel::mpsc::Receiver<Box<Send + 'static>> = None.unwrap();

    await!(chan.next());
}

As futures::channel::mpsc::Receiver has an impl of Send+Sync when its generic parameter is Send, and Box<Send + 'static> is Send, I would expect the future returned by this function to be Send. However, when trying to require it, I get:

error[E0277]: `dyn std::marker::Send` cannot be sent between threads safely
  --> erlust/src/lib.rs:30:9
   |
30 |         spawn(__receive());
   |         ^^^^^ `dyn std::marker::Send` cannot be sent between threads safely
   |
   = help: the trait `for<'r> std::marker::Send` is not implemented for `dyn std::marker::Send`
   = note: required because of the requirements on the impl of `for<'r> std::marker::Send` 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 `for<'r> std::marker::Send` for `futures::channel::mpsc::Inner<std::boxed::Box<dyn std::marker::Send>>`
   = note: required because of the requirements on the impl of `for<'r> std::marker::Send` for `std::sync::Arc<futures::channel::mpsc::Inner<std::boxed::Box<dyn std::marker::Send>>>`
   = note: required because it appears within the type `futures::channel::mpsc::Receiver<std::boxed::Box<dyn std::marker::Send>>`
   = note: required because it appears within the type `for<'r, 's, 't0> {futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 'r)>>, futures::stream::Next<'s, futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 't0)>>>, ()}`
   = note: required because it appears within the type `[static generator@erlust/src/lib.rs:18:1: 22:2 for<'r, 's, 't0> {futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 'r)>>, futures::stream::Next<'s, futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 't0)>>>, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@erlust/src/lib.rs:18:1: 22:2 for<'r, 's, 't0> {futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 'r)>>, futures::stream::Next<'s, futures::channel::mpsc::Receiver<std::boxed::Box<(dyn std::marker::Send + 't0)>>>, ()}]>`
   = note: required because it appears within the type `impl futures::Future`
   = note: required because it appears within the type `impl futures::Future`

I am not 100% sure this issue is actually tied to async/await, but removing the await! makes the issue vanish, so… cc #50547

A full reproducer can be found here, but unfortunately doesn't compile on the playground due to lack of futures-0.3.

@Nemo157
Copy link
Member

Nemo157 commented Aug 11, 2018

Simpler repro using generators directly (playground):

fn bar() -> impl Generator + Send {
    || {
        let bar: Box<dyn Send + 'static> = Box::new(5);
        yield;
        drop(bar)
    }
}
5 | fn bar() -> impl Generator + Send {
  |             ^^^^^^^^^^^^^^^^^^^^^ `dyn std::marker::Send` cannot be sent between threads safely
  |
  = help: the trait `for<'r> std::marker::Send` is not implemented for `dyn std::marker::Send`
  = note: required because of the requirements on the impl of `for<'r> std::marker::Send` 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 it appears within the type `for<'r> {std::boxed::Box<(dyn std::marker::Send + 'r)>, ()}`
  = note: required because it appears within the type `[generator@src/main.rs:6:5: 10:6 for<'r> {std::boxed::Box<(dyn std::marker::Send + 'r)>, ()}]`

It seems the issue is that dyn Send is not for<'r> Send, but I have no idea why that is.

@elinorbgr
Copy link
Contributor

Interestingly, this simpler example compiles fine:

fn need_send<T: ?Sized + Send>() {}

fn main() {
    need_send::<dyn Send>();
}

So dyn Send is actually Send, but in the context of generators, rustc seems to fail to realize that?

@withoutboats
Copy link
Contributor

withoutboats commented Aug 13, 2018

@Nemo157's error is very surprising, since I thought @cramertj had correctly identified the error here on the tracking issue:

std::mpsc::Receiver isn't Sync, and the async fn you wrote holds a reference to it. References to !Sync items are !Send.

That is, I thought a reference to receiver was being stored in its state because of the way the await!(chan.next()) was being expanded. I've been vaguely afraid that this problem would crop up: internal references in async fn should be Send because they're safely encapsulated here. I'm not sure how often this will appear as a problem, so far no one's reported it (unless that's what underlies this issue, but it doesn't look like it).

However, @Nemo157's is much more surprising dyn Send certainly does implement Send; a core part of being an object-safe trait is that dyn Trait: Trait holds. It's certainly a bug.

@Ekleog
Copy link
Author

Ekleog commented Aug 13, 2018

For the record, futures::channel::mpsc::Receiver is Send+Sync when the wrapped type is Send, even though the std one isn't. So that problem hopefully shouldn't crop up in this specific case at least :)

@Ekleog
Copy link
Author

Ekleog commented Aug 21, 2018

Potentially related issue: #53548 (has the same for<'r> bound appearing out of seemingly nowhere)

@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-coroutines Area: Coroutines A-async-await Area: Async & Await labels Jan 27, 2019
@nikomatsakis
Copy link
Contributor

Does not reproduce if you don't use dyn Send. For example this works (playground):

#![feature(generators, generator_trait)]

use std::ops::Generator;

trait Foo: Send { }

impl Foo for u32 { }

fn bar() -> impl Generator + Send {
    || {
        let bar: Box<dyn Foo + 'static> = Box::new(5);
        yield;
        drop(bar)
    }
}

fn main() {}

@nikomatsakis
Copy link
Contributor

Using dyn Foo + Send also works -- @Ekleog can you share a bit of where this example came from?

@Ekleog
Copy link
Author

Ekleog commented Feb 22, 2019

@nikomatsakis Looks like this issue has been fixed! It's one of the few issues I raised while trying to get code using a macro that expands to this to generate code that builds correctly. Basically, I've been minimizing the example to come up with this specific issue.

As another minimized error was #53548 that isn't fixed yet I'd assume the overall code doesn't build yet, but I'll make sure to open another issue if I can minimize the original example to another reasonably small code chunk. I think this issue can be closed for now. Thank you! :)

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-coroutines Area: Coroutines C-bug Category: This is a bug. 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

6 participants