Allow ? in routes #431

sinistersnare · 2020-05-16T16:37:14Z

Hi, I want to write code like this:

fn homepage(state: State) -> search::Result<(State, Response<Body>)> {
    let a = expr()?
    return (state, a);
}

fn make_router() -> search::Result<Router> {
    let state = IndexState::new()?;
    let middleware = StateMiddleware::new(state);
    let pipeline = single_middleware(middleware);
    let (chain, pipelines) = single_pipeline(pipeline);
    Ok(build_router(chain, pipelines, |route| {
        route.get("/").to(homepage); // FIXME! doesnt work!
    }))
}

But the IntoHandlerFuture trait does not get auto-impld for homepage. Therefore, I cant use ? in my function body.

I tried adding the following into gotham:

impl<T: IntoHandlerFuture, E: IntoHandlerFuture> IntoHandlerFuture for std::result::Result<T, E> {
    fn into_handler_future(self) -> Pin<Box<HandlerFuture>> {
        match self {
            Ok(hf) => hf.into_handler_future(),
            Err(hf) => hf.into_handler_future(),
        }
    }
}

But I cant impl IntoHandlerFuture for my Error type:

impl IntoHandlerFuture for Error {
    fn into_handler_future(self) -> Pin<Box<HandlerFuture>> {
        future::ok((NO_STATE_AVAILABLE_HERE, response)).boxed()
    }
}

But there was no way to get the state to return in into_handler_future.

So I ask, can the API be changed to allow such code? Thank you.

The text was updated successfully, but these errors were encountered:

msrd0 · 2020-05-17T15:33:24Z

I believe what you can do to work around this limitation is something like this:

fn homepage(state: &State) -> search::Result<Response<Body>> {
    let a = expr()?;
    return a;
}

fn wrap_handler<F>(callback: F, state: State) -> Pin<Box<HandlerFuture>>
where
	F : FnOnce(&State) -> Response<Body>
{
	let res = match callback(&state) {
		Ok(res) => future::ok((state, res)).boxed(),
		Err(err) => future::err((state, res.into_handler_error())).boxed()
	};
}

fn make_router() -> search::Result<Router> {
    let state = IndexState::new()?;
    let middleware = StateMiddleware::new(state);
    let pipeline = single_middleware(middleware);
    let (chain, pipelines) = single_pipeline(pipeline);
    Ok(build_router(chain, pipelines, |route| {
        route.get("/").to(|state| wrap_handler(homepage, state));
    }))
}

This allows you some usage of the ? operator while still giving gotham its State back. Note that this will not work when you use &State from an async context.

I am not one of the maintainers but I still believe that there is a good reason for gotham to require the State back from the handler and to not have it implement Sync (that would allow to pass &mut State instead of State to the handler, eliminating the need to return it, and not restricting anything as it can't be consumed anyways).

Also you might want to take a look at the Handler documentation or the examples.

alsuren · 2020-05-23T22:08:55Z

Historically, the future combinators were not able to express borrowing very well. The solution of passing things by move worked out pretty well in that era. The language is now capable of expressing lifetimes of things borrowed across await points, so there are a bunch of wrappers/macros floating about in the ecosystem that allow you to receive async fn arguments as &mut. Unfortunately, the ergonomics of this are pretty poor.

When I was implementing .to_async() in #281 I thought about making a .to_async_borrowing() router method. I didn't have any trouble with Sync, but I did have trouble with the higher-ranked lifetime bounds that are required to borrow in an async context.

I've now stumbled across this problem in a second project, so I'm going to try fixing it. This will probably involve patches to the compiler and take many months though.

msrd0 · 2020-05-23T23:14:00Z

@alsuren I also tried to fix it but failed. What I attempted so far while writing gotham-restful were two things:

Take the State synchronously and pass &State into the async context, like |state| dosth(&state), with async fn dosth(state: &State). This is impossible because rustc insists on &State to have static lifetime, which obviously is not the case.
Take the State into async context and pass it as a reference, like |state| async move { dosth(&state).await }. This is impossible because State does not implement Sync.

As of right now I believe there is no good solution, so I generate a compile-time error if the user code tries to use async together with the state. However, I'd definitely appreciate a better solution for this.

technic · 2020-06-06T22:40:54Z

I am wondering how does actix handle this situation. Or doest it at all? In any case using ? operator would be convenient. Maybe it is impossible in all cases, but I think we can have few wrappers.
In my current code I often first generate response and then mix the state in the end. In order to use ? I need to wrap the function body into some lambda. This would be easier to do when rust will get try block.

technic · 2020-06-11T10:15:29Z

Hi, I was playing around with this. I want to write handlers in the following fashion:

pub async fn handler(state: &mut State) -> Result<&'static str, HandlerError> {
    let b = body::to_bytes(Body::take_from(state)).await.map_err(bad_request)?;
    let s = std::str::from_utf8(&b).map_err(bad_request)?;
    match s.find("text") {
        Some(_) => Ok("Found"),
        None => Ok("Bye!"),
    }
}

So I can use await and ? syntax without extra boiler plate.

It turns out, that this is possible to achieve with the following code, when using lambdas

route.get("/example").to(|mut st: State| {
    async move {
        let result = handler(&mut st).await;
        to_handler_result(st, result)
    }
    .boxed()
});

So far I failed to write Handler implementation for such async fn or to write any wrapper.
It looks like rust compiler is smart enough to deduce correct lifetimes in lambdas, but not in the generic code.

technic · 2020-06-29T12:28:33Z

Thanks to the solution on the rust forum I am finally able to have this work also for async fn.

The trick is to define helper trait. Then the code above will work like this

route.get("/example").to(SimpleAsyncHandler(handler));

So I think you can do to_async_borrowing() with this.

@alsuren, can you explain what was the problem with to_async_borrowing()? Maybe required changes have already landed into compiler?

msrd0 · 2020-06-29T19:58:34Z

@technic Can you share the code for SimpleAsyncHandler? I tried to put your code above into a to_async_borrowing method but the compiler is not happy:

self.to_new_handler(move || Ok(move |s: State| {
	async move {
		let res = handler(&s).await;
		to_handler_result(s, res)
	}.boxed()
}));

gives me the following error:

error: future cannot be sent between threads safely
   --> gotham/src/router/builder/single.rs:564:6
    |
564 |             }.boxed()
    |               ^^^^^ future created by async block is not `Send`
    |
    = help: the trait `std::marker::Sync` is not implemented for `(dyn std::any::Any + std::marker::Send + 'static)`
note: future is not `Send` as this value is used across an await
   --> gotham/src/router/builder/single.rs:562:15
    |
562 |                 let res = handler(&s).await;
    |                           ^^^^^^^^--^^^^^^^- `&s` is later dropped here
    |                           |       |
    |                           |       has type `&state::State` which is not `Send`
    |                           await occurs here, with `&s` maybe used later
help: consider moving this into a `let` binding to create a shorter lived borrow
   --> gotham/src/router/builder/single.rs:562:15
    |
562 |                 let res = handler(&s).await;
    |                           ^^^^^^^^^^^

So the compiler is asking for &State: Send (or State: Sync respectively) which is not implemented for State. I've used rust/cargo 1.44.0, maybe this will be possible in an upcoming rust version only?

sezna · 2020-08-26T13:13:50Z

For anybody referring to this issue, currently a handler wrapper would be required to handle this.

I think moving forward, as the Rust ecosystem has been developing, it would be worth discussing to_async actually being the default, and to_sync being the exception. Then, implementing referential State transfer and using the ? syntax in handlers would be a much higher priority for async handlers. I think this is out of scope for 0.5.

msrd0 · 2020-08-26T13:15:17Z

I'm not so sure, at least #450 sounds very promising in that regard (although is being blocked unless #438 is merged)

sezna · 2020-08-26T13:19:39Z

Ah, didn't realize those were close. This could be rolled into 0.5 then, looking at those PRs. Would you agree?

msrd0 · 2020-08-26T13:20:35Z

Yes, I think this could be included in 0.5

alsuren mentioned this issue May 23, 2020

Pass State in as a reference alsuren/gotham#3

Closed

sezna added discussion specification labels Aug 26, 2020

sezna added this to the 0.6 milestone Aug 26, 2020

msrd0 modified the milestones: 0.6, 0.5 Aug 26, 2020

msrd0 linked a pull request Sep 4, 2020 that will close this issue

Async borrowing (continuation of #449) #450

Merged

4 tasks

msrd0 closed this as completed in #450 Sep 5, 2020

alsuren mentioned this issue Mar 26, 2021

Async-closures or reference-parameterized-closures-returning-futures rust-lang/wg-async#78

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ? in routes #431

Allow ? in routes #431

sinistersnare commented May 16, 2020

msrd0 commented May 17, 2020

alsuren commented May 23, 2020

msrd0 commented May 23, 2020

technic commented Jun 6, 2020

technic commented Jun 11, 2020 •

edited

Loading

technic commented Jun 29, 2020

msrd0 commented Jun 29, 2020

sezna commented Aug 26, 2020

msrd0 commented Aug 26, 2020

sezna commented Aug 26, 2020

msrd0 commented Aug 26, 2020

Allow ? in routes #431

Allow ? in routes #431

Comments

sinistersnare commented May 16, 2020

msrd0 commented May 17, 2020

alsuren commented May 23, 2020

msrd0 commented May 23, 2020

technic commented Jun 6, 2020

technic commented Jun 11, 2020 • edited Loading

technic commented Jun 29, 2020

msrd0 commented Jun 29, 2020

sezna commented Aug 26, 2020

msrd0 commented Aug 26, 2020

sezna commented Aug 26, 2020

msrd0 commented Aug 26, 2020

technic commented Jun 11, 2020 •

edited

Loading