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

not Send due to await retainment #67611

Open
Mark-Simulacrum opened this issue Dec 25, 2019 · 15 comments
Open

not Send due to await retainment #67611

Mark-Simulacrum opened this issue Dec 25, 2019 · 15 comments

Comments

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Dec 25, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 2, 2020

triage: P-high. Nominating for discussion.

@pnkfelix pnkfelix self-assigned this Jan 2, 2020
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 2, 2020

self-assigned for investigation in short term (as in, next two or three days). However I suspect someone from @rust-lang/wg-async-await will be a better choice to investigate this.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 2, 2020

We should try to bisect this

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 3, 2020

used cargo-bisect-rustc.

Determined:

searched nightlies: from nightly-2019-11-26 to nightly-2020-01-03
regressed nightly: nightly-2019-12-01
searched commits: from 25d8a94 to d8bdb3f
e1a6669236122c41ed2aed3
regressed commit: 9081929

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 3, 2020

9081929 is a rollup commit of the following PR's:


 - #66379 (Rephrase docs in for ptr)
 - #66589 (Draw vertical lines correctly in compiler error messages)
 - #66613 (Allow customising ty::TraitRef's printing behavior)
 - #66766 (Panic machinery comments and tweaks)
 - #66791 (Handle GlobalCtxt directly from librustc_interface query system)
 - #66793 (Record temporary static references in generator witnesses)
 - #66808 (Cleanup error code)
 - #66826 (Clarifies how to tag users for assigning PRs)
 - #66837 (Clarify `{f32,f64}::EPSILON` docs)
 - #66844 (Miri: do not consider memory allocated by caller_location leaked)
 - #66872 (Minor documentation fix)

I'm guessing this is due to PR #66793

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 6, 2020

I'm still working on reducing this down to something that I could put into the playpen.

However, one interesting thing I've found locally during my reduction:

This version of the fn proxy will signal an analogous compilation error:

async fn proxy(buf: &[u8]) -> Result<Vec<u8>> {
    let tv = unsafe { TIMEOUT };
    let dur = Duration::from_millis(unsafe { TIMEOUT });
    timeout(Duration::from_millis(unsafe { TIMEOUT }),
            async { Result::Ok(vec![0u8]) })
        .await?
        ;

    Result::Ok(vec![0u8])
}

but if I lift the first argument out to a separate let-binding (i.e. change it to use dur above, or the sub-expression unsafe { TIMEOUT } out to a separate let-binding (i.e. change the argument expression to use tv above), then the error goes away.

I'll admit that I don't know all the details of how the fn-transformation works to support async fn, but I strongly suspect that if this kind of local code change can make an error go away, then that represents a weakness somewhere (either in the fn-transformation, or in the borrowck).

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 6, 2020

Okay in the details block is a standalone version suitable for the playground:

#![crate_type="lib"]

use std::future::Future;
use std::io::Result;
use std::pin::Pin;
use std::task::{Context, Poll};
use std::time::Duration;

pub fn timeout<T>(_duration: Duration, _future: T) -> Timeout<T>
where
    T: Future,
{
    loop { }
}

#[derive(Debug)]
pub struct Elapsed(());

impl std::fmt::Display for Elapsed {
    fn fmt(&self, w: &mut std::fmt::Formatter) -> std::fmt::Result {
        write!(w, "Elapsed")
    }
}

impl std::error::Error for Elapsed {}

impl From<Elapsed> for std::io::Error {
    fn from(_err: Elapsed) -> std::io::Error {
        std::io::ErrorKind::TimedOut.into()
    }
}

pub struct Timeout<T> {
    _value: T,
}

impl<T> Future for Timeout<T>
where
    T: Future,
{
    type Output = std::result::Result<T::Output, Elapsed>;

    fn poll(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll<Self::Output> {
        loop { }
    }
}

static mut TIMEOUT: u64 = 2000;

use std::marker::PhantomData;

pub struct JoinHandle<T> {
    _p: PhantomData<T>,
}

pub fn spawn<T>(_task: T) -> JoinHandle<T::Output>
where
    T: Future + Send + 'static,
    T::Output: Send + 'static,
{
    loop { }
}

pub async fn my_main() {
    spawn(proxy());
}

// This version of `fn proxy` demonstrates the error
pub async fn proxy() -> Result<Vec<u8>> {
    timeout
        (Duration::from_millis(unsafe { TIMEOUT }),
         async { Result::Ok(vec![0u8]) })
        .await?
        .unwrap()
        ;

    Result::Ok(vec![0u8])
}

// This variant does not generate an error
pub async fn proxy_2() -> Result<Vec<u8>> {
    let dur = Duration::from_millis(unsafe { TIMEOUT });

    timeout
        (dur,
         async { Result::Ok(vec![0u8]) })
        .await?
        .unwrap()
        ;

    Result::Ok(vec![0u8])
}

// This variant does not generate an error
pub async fn proxy_3() -> Result<Vec<u8>> {
    let tv = unsafe { TIMEOUT };

    timeout
        (Duration::from_millis(tv),
         async { Result::Ok(vec![0u8]) })
        .await?
        .unwrap()
        ;

    Result::Ok(vec![0u8])
}

and the error it generates:

error: future cannot be sent between threads safely
  --> src/lib.rs:65:5
   |
56 | pub fn spawn<T>(_task: T) -> JoinHandle<T::Output>
   |        -----
57 | where
58 |     T: Future + Send + 'static,
   |                 ---- required by this bound in `spawn`
...
65 |     spawn(proxy());
   |     ^^^^^ future returned by `proxy` is not `Send`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `*mut u64`
note: future is not `Send` as this value is used across an await
  --> src/lib.rs:70:5
   |
70 | /     timeout
71 | |         (Duration::from_millis(unsafe { TIMEOUT }),
   | |                                         ------- has type `*mut u64`
72 | |          async { Result::Ok(vec![0u8]) })
73 | |         .await?
   | |______________^ await occurs here, with `TIMEOUT` maybe used later
74 |           .unwrap()
75 |           ;
   |           - `TIMEOUT` is later dropped here

error: aborting due to previous error
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 6, 2020

So this does seem like somewhat clear (expected?) fallout from PR #67611: a use of a static mut X: T in a generator witness will be recorded as a unsafe pointer *mut T, and then *mut T is never Send, but the spawn method requires that the task you give it is Send.

I believe we can readily update the crate in question to avoid using the static mut in the expression under the RHS of await.

But I do wonder whether we can/should improve the diagnostics here to try to guide the user towards such a solution ...?

@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 6, 2020

I believe we can readily update the crate in question to avoid using the static mut in the expression under the RHS of await.

I might have worded this too strongly, and/or over-reduced my example.

In particular, its easy to move around the access to TIMEOUT: u64. But the borrow of PROXY: Vec<SocketAddr> may be trickier to deal with.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

@matthewjasper matthewjasper commented Jan 6, 2020

I'll have a look at addressing this with another approach I considered for #66793. If that doesn't work out then I'll have a look at the error message.

@matthewjasper matthewjasper self-assigned this Jan 6, 2020
@pnkfelix

This comment has been minimized.

Copy link
Member

@pnkfelix pnkfelix commented Jan 6, 2020

I believe we can readily update the crate in question to avoid using the static mut in the expression under the RHS of await.

I might have worded this too strongly, and/or over-reduced my example.

In particular, its easy to move around the access to TIMEOUT: u64. But the borrow of PROXY: Vec<SocketAddr> may be trickier to deal with.

The patch linked is enough to fix the crate in question, but I'm all for @matthewjasper finding an in-compiler solution if we can do it soundly.

https://gist.github.com/pnkfelix/4bdef50d0538fee8f6e0cbc4379f6074

@matthewjasper

This comment has been minimized.

Copy link
Contributor

@matthewjasper matthewjasper commented Jan 7, 2020

OK. I'm going to write some things up because I've changed my mind a bunch of times about how/if this can be fixed in MIR building and I don't want to keep forgetting things.

Background: auto traits and generators

Generators store values that if their value needs to be preserved across a yield. These values need to be visible for auto-traits, for example a generator may be resumed on a different thread after it yields, and so cannot be Send if any of theses values isn't Send.

There are two passes that determine which values can be included in the generator:

  • A pass that runs as part of typeck that walks the HIR to find the types of expressions and then checks for yields in the values scope (as determined by the RegionScopeTree). The types recorded here are stored in the GeneratorWitnesss type contained in the generator, which is then used for auto trait checking. This notably uses the types of expressions that HIR sees.
  • The MIR transform that lowers the generator to a state machine. This determines the values that the generator actually stores. As a check for the compiler, we assert that any types that we capture here are included in the witness type (we don't know all auto traits, so can't check only for them).

Background: MIR lowering of statics.

After #66587 statics no longer have a special representation in MIR. They are instead lowered to a dereference of a pointer to the given static:

static X: T = ...;
static mut Y: S = ...;
// ...
use(X);
use(Y);

is lowered as if it were:

static X: T = ...;
static mut Y: S = ...;
const X_PTR: &T = &X; // Imagine that this could compile
const Y_PTR: *mut T = &raw mut Y; // Imagine that this doesn't need an unsafe block
// ...
use(*X_PTR);
use(*Y_PTR);

Which becomes:

_tmp1 = const &X;
use(*_tmp1);
_tmp2 = const &raw mut Y;
use(*_tmp2);

The first problem

The example above is somewhat deceptive in that the temporary that holds the pointer is used immediately after it's assigned. In this case it cannot end up in the generator since it's not live across any user code. However this isn't true in all cases, index and match expressions allow running user code while the pointer is live:

X[EXPR]
// Becomes
_tmp1 = const &X;
_tmp2 = EXPR;
// bounds check
(*_tmp1)[_tmp2]

match Y { a if GUARD => { /* ... */ }
// Becomes
_tmp3 = const &raw mut Y;
// check guard
a = *_tmp3;

In these cases MIR has to put the temporary pointer into the generator state and promptly ICEs.

Fixing the ICE

The obvious solution now is make type checking record the new pointer types in the generator witness in these cases. Unfortunately HIR is not a great tool for specifying "these cases". The methods that the region scope tree provides in HIR are either usually far too large (the "temporary scope") or occasionally too small (the "enclosing scope"). #66793 went for the sound but rather approximate approach, leaving us here.

Other potential fixes

Change MIR building

One idea might be to try changing MIR building so that the temporary is never live across user code. For example, one might change the lowering as follows

X[EXPR]
// Becomes
_tmp2 = EXPR;
// bounds check
_tmp1 = const &X;
(*_tmp1)[_tmp2]

However working out how to do this for match in a reasonable to implement way is harder.

Internal

MIR actually already contains an escape hatch for values that are in the MIR but aren't in the HIR in the form of a flag (called internal) on the variable. We currently use it for drop flags and boxes created by box expressions. It's unclear what exactly the difference is between

static Y = // ...

let x = &Y;
yield;
use(x);
// and
let x = &Y;
yield;
use(&Y);

is. I guess one could argue that auto traits are for layout introspection and this would be further breaking that.

I think that's everything for now.

@tmandry

This comment has been minimized.

Copy link
Contributor

@tmandry tmandry commented Jan 14, 2020

Visiting from triage. @matthewjasper were you planning to work on this further?

@tmandry

This comment has been minimized.

Copy link
Contributor

@tmandry tmandry commented Jan 14, 2020

It's unclear what exactly the difference is between

static Y = // ...

let x = &Y;
yield;
use(x);
// and
let x = &Y;
yield;
use(&Y);

It's definitely tempting to take advantage of the constraints around statics to just declare these equivalent, and use the internal flag to get out of this. While I don't see any soundness issues around that, I also don't quite trust myself to get this right :)

I guess one could argue that auto traits are for layout introspection and this would be further breaking that.

Can you expand on this? I'm not sure I follow.

@tmandry

This comment has been minimized.

Copy link
Contributor

@tmandry tmandry commented Jan 14, 2020

Another solution that comes to mind relies on the fact that these pointers never need to be stored in a generator. They are consts, and the temporaries containing them shouldn't ever be mutated.

We could add semantics for "const temporaries" to MIR, and never store them inside generators. The semantics would (hopefully) be simple: defined once and assigned a value that doesn't depend on any other temporaries; never mutated or borrowed mutably. We can move the statements defining them to the top of the resume function inside the generator transform.

But that would be adding more semantics and special casing to MIR to fix a change which removed extra semantics and special casing, so I'm not sure if that's what we want :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.