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

Relax lifetime bound for &Request in respond_to (async branch) #1334

Closed
wants to merge 2 commits into from

Conversation

jebrosen
Copy link
Collaborator

This was reported in IRC: https://freenode.logbot.info/rocket/20200525#c3970295 - see also GREsau/okapi#29.

Currently, respond_to is too restrictive: it requires an &'r Request, where 'r is from the trait - so if you have an R: Responder<'static> (i.e. an owned Responder), you can only respond_to an &'static Request. This PR changes that.

Before merging, I would also want to find more concrete documentation of using 'async_trait in this position. So far, the closest I have found is dtolnay/async-trait#8 (comment).

jebrosen added 2 commits June 10, 2020 16:32
…r<'static>`.

Specifically, the respond_to method now takes an `&Request<'_>` instead
of an `&'r Request<'_>`.
@jebrosen jebrosen changed the title Responder lifetime issue Relax lifetime bound for &Request in respond_to (async branch) Jun 10, 2020
@SergioBenitez
Copy link
Member

This is...weird. A magical lifetime that appears out of nowhere! I'm beginning to wonder if we should have our own #[async_trait] that's aware of trait names and "does the right thing" with lifetimes. Or perhaps using #[async_trait] is simply not a good idea, given just how many workaround we need.

@jebrosen
Copy link
Collaborator Author

Or perhaps using #[async_trait] is simply not a good idea, given just how many workaround we need.

I think that was my original conclusion at least for this trait, which is in the unique position of having a lifetime that applies to self and the return value but no other parameters. I was excited to see when you applied it to Responder, thinking the problems I ran into had been resolved - but it looks like those issues were only moved somewhere else in this case. For what it's worth, rustc might in principle be able to do this without the additional help; I believe #[async_trait] truly needs it here because the generated code for the impl block can't depend in any way on the original trait definition.

As far as I can tell the same concerns do not apply to Fairing and FromRequest, so it is still a net win to use #[async_trait] for those.

@SergioBenitez SergioBenitez self-assigned this Jun 18, 2020
@SergioBenitez
Copy link
Member

SergioBenitez commented Jun 18, 2020

Update: we've decided to make Responder synchronous with the rationale that:

  1. Most Responders are synchronous, with the exception of using Response::sized_body().
  2. We can make set_sized_body synchronous by differing the Seek operation to just before Rocket writes out the response.
  3. Constructors for types that implement Responder constructors can be async if they need be.

I'll be assuming responsibility for these changes. I'll keep this PR open until I've pushed my own.

@SergioBenitez
Copy link
Member

Changes merged in 4f79255.

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Jun 22, 2020
@jebrosen jebrosen deleted the responder-lifetime-issue branch June 22, 2020 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants