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

Handle borrowed arguments #11

Closed
alex opened this Issue Aug 31, 2017 · 16 comments

Comments

Projects
None yet
8 participants
@alex
Copy link
Contributor

alex commented Aug 31, 2017

(The README alludes to ongoing work/thought on this. I figured there should be an issue to track it.)

Right now, borrowed arguments in #[async] functions are no bueno. Life would be easier if they were supported, as the alternate is to Box arguments or somehow convert them to owned.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Aug 31, 2017

as the alternate is to Box arguments or somehow convert them to owned.

The better alternative is probably to use async_block!, since that allows annotating the function return type with the lifetime constraints (impl Future<...> + 'a etc). Seems to work for me, atleast.

Edit: That is, this doesn't compile:

#[async] fn foo(x: &Bar) -> Result<&Baz, ()> {
    Ok(await!(...))
}

but this does:

fn foo<'a>(x: &'a Bar) -> impl Future<Item = &'a Baz, Error = ()> + 'a {
    async_block! {
        Ok(await!(...))
    }
}
@rushmorem

This comment has been minimized.

Copy link

rushmorem commented Aug 31, 2017

The better alternative is probably to use async_block!, since that allows annotating the function return type with the lifetime constraints (impl Future<...> + 'a etc)

Maybe this should be included in the README. @alexcrichton Any issues with this approach?

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Aug 31, 2017

Thanks for opening a tracking issue @alex!

I think we can probably support the case brought up by @Arnavion by tweaking the procedural macro (taking into account lifetime arguments or otherwise not mentioning 'static here.

One worry I'd have though is that cases like @Arnavion brought up are quite rare, and otherwise requiring 'static would hopefully lead to better error messages, although I'm not sure if it's actually worth it?

@alex

This comment has been minimized.

Copy link
Contributor Author

alex commented Aug 31, 2017

FWIW, I found the technique @Arnavion suggested pretty effective at resolving some lifetime issues that I was otherwise having trouble with.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Aug 31, 2017

One worry I'd have though is that cases like @Arnavion brought up are quite rare

Yeah, that's possible. The project I'm trying futures_await on is a CLI program that creates an HTTP Client, runs the event loop, then exits. Since the Client outlives the event loop I just have all the functions take &'a Client and return futures bounded by 'a. I can see the scenario where Self isn't guaranteed to outlive all the futures, so all the functions would work on Rc<Self> and return 'static futures as you say in the README.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Aug 31, 2017

BTW another advantage of async_block! over #[async] is when you don't need some borrows to last as long as the returned Future. Eg:

fn post<'a>(client: &'a Client, body: &Body) -> impl Future<...> + 'a {
    let builder = client.inner_http_client.post(body);
    async_block! {
        let response = await!(builder.send())?;
        let result = response.deserialize()?;
        Ok(result)
    }
}

post() synchronously serializes the &Body into the returned builder, so builder doesn't borrow from the &Body (see the actual post function here). By keeping the use of the &Body outside the async_block it doesn't need to have an 'a bound, since it doesn't need to live as long as the returned Future.

I don't know if there's any way to make #[async] smart enough to make this work.

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Aug 31, 2017

@Arnavion

BTW another advantage of async_block! over #[async] is when you don't need some borrows to last as long as the returned Future. Eg:

Indeed!

I'd love to play around with ideas about how to make this sort of pattern more ergonomic, although I haven't quite figured out how to do so yet :(

@alex

This comment has been minimized.

Copy link
Contributor Author

alex commented Aug 31, 2017

async_block! isn't too bad in my view. My personal assessment is that the biggest win is making some common scenarios with borrows work better with #[async].

@daaku

This comment has been minimized.

Copy link

daaku commented Oct 2, 2017

Would be good to point folks to this workaround. It wasn't discoverable, but immediately solved my problem. I understand it may not be the best long term solution but it certainly helps for now.

@manuels

This comment has been minimized.

Copy link

manuels commented Oct 12, 2017

I really like the approach by @Arnavion but I am running into a problem when working with &mut self.

#![feature(conservative_impl_trait, generators, proc_macro)]
extern crate futures_await;

use futures_await as futures;
use futures::prelude::*;
use futures::Future;

struct Foo { i: u8 }

impl Foo {
    fn increase<'a>(&'a mut self) -> impl Future<Item=u8, Error=()> + 'a {
        async_block! {
            self.i += 1;
            Ok(self.i)
        }
    }
}

fn run<'a>(foo: &'a mut Foo) -> impl Future<Item=u8, Error=()> + 'a {
    async_block! {
        // await!(foo.increase())?; // compiler fails if this line is included!
        await!(foo.increase())
    }
}

fn main() {
    let mut foo = Foo { i: 0 };
    run(&mut foo).wait().unwrap();
}

This code works fine if I just do only one call to foo.increase() but when including the second call to foo.increase() I get:

error[E0499]: cannot borrow `*foo` as mutable more than once at a time
  --> main.rs:22:16
   |
21 |         await!(foo.increase())?;
   |                --- first mutable borrow occurs here
22 |         await!(foo.increase())
   |                ^^^ second mutable borrow occurs here
23 |     }
   |     - first borrow ends here

I do not see why the first borrow ends at the end of the function call (line 23), or am I wrong?
Is this the borrow checker's fault or is there something wrong in the futures_await macros?

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Oct 12, 2017

@manuels Neither. It appears to be a bug in generators arising because Foo::increase returns a value bounded by self's lifetime and there is a yield point while it's borrowed. I reduced it to this:

#![feature(conservative_impl_trait, generators, generator_trait)]

extern crate futures;

use std::ops::{ Generator, GeneratorState };
use futures::{ Async, future, Future };

struct Foo {
    i: u8,
}

impl Foo {
    fn i<'a>(&'a mut self) -> impl Future<Item = u8, Error = ()> + 'a {
        future::ok(self.i)
    }
}

fn run<'a>(foo: &'a mut Foo) -> impl Generator<Yield = (), Return = Result<u8, ()>> + 'a {
    move || {
        if false {
            yield
        }

        let _ = {
            let mut f = foo.i();
            loop {
                let poll = f.poll();
                match poll {
                    Ok(Async::Ready(i)) => break i,
                    Ok(Async::NotReady) => yield,
                    _ => unreachable!(),
                }
            }
        };

        {
            let mut f = foo.i();
            let poll = f.poll();
            match poll {
                Ok(Async::Ready(i)) => Ok(i),
                _ => unreachable!(),
            }
        }
    }
}

fn main() {
    let mut foo = Foo { i: 0 };
    let mut g = run(&mut foo);
    match g.resume() {
        GeneratorState::Complete(Ok(i)) => println!("{}", i),
        _ => unreachable!(),
    }
}

(It can probably be simplified much more by getting rid of the futures crate, but I don't have the time atm.)

Either removing the Ok(Async::NotReady) => yield, line or changing the bound of the result of Foo::i to 'static get rid of the error.

@Arnavion

This comment has been minimized.

Copy link

Arnavion commented Nov 8, 2017

The equivalent workaround for #[async_stream] is async_stream_block! (not documented in the README but exposed from futures::prelude just the same):

fn foo<'a>() -> impl Stream<...> + 'a {
    async_stream_block! {
        /* Whatever you would've written inside an #[async_stream] function */
    }
}
@thedodd

This comment has been minimized.

Copy link

thedodd commented Dec 18, 2017

Outstanding work on all of this!

In the name of getting some feedback out there, in respect to ergonomics, I would say that having references supported in #[async] functions is a must.

When creating libraries or simply creating internal types, encapsulation is going to lead to interfaces with lots of &self & a bit of &mut self for accessing config & other various types which have been encapsulated by the parent type. Having to rely on associated functions or async_block! for creating async interfaces ... not super awesome.

So, where exactly does the limitation come from at this point? Is this an issue with futures, the generator/coroutine implementation, or just the macros themselves?

Once again, outstanding work on this! Keep up the good work.

@manuels

This comment has been minimized.

Copy link

manuels commented Jan 23, 2018

The nasty bug for the workaround is fixed if you use #![feature(nll)]!

@withoutboats

This comment has been minimized.

Copy link
Collaborator

withoutboats commented Feb 18, 2018

PR #63 will resolve this issue, including support for elided lifetimes.

@withoutboats

This comment has been minimized.

Copy link
Collaborator

withoutboats commented Mar 11, 2018

Closing because it should be fixed in 0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment