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

Convert actix-web extractors to async await #544

Closed
fzzzy opened this issue Mar 25, 2020 · 10 comments
Closed

Convert actix-web extractors to async await #544

fzzzy opened this issue Mar 25, 2020 · 10 comments
Assignees
Labels
5 Estimate - l - Moderately complex, will require some effort but clearly defined. good first bug Low priority, but valuable contributions sync-modernization

Comments

@fzzzy
Copy link
Contributor

fzzzy commented Mar 25, 2020

In the FromRequest implementations for BsoBodies and BsoBody, we invoke and_then() on the Future returned by another from_request() invocation. Now that Rust supports async/await, this is no longer idiomatic. Instead, we should wrap the from_request() invocation in a Box::pin(async move { ... }) block as we do in the other extractors. This would allow us to use async/await.

@fzzzy fzzzy added this to Backlog: Sync in Services Engineering Mar 25, 2020
@fzzzy fzzzy added 5 Estimate - l - Moderately complex, will require some effort but clearly defined. good first bug Low priority, but valuable contributions labels Mar 25, 2020
@Emmanuel-Melon
Copy link
Contributor

@fzzzy, I'd like to work on this issue.

@Emmanuel-Melon
Copy link
Contributor

Emmanuel-Melon commented Apr 10, 2020

@fzzzy Since Rust doesn't support Async in traits, can we utilize one of the following approaches

  • Async Trait
  • I've looked into the docs and we can desugar this by returning a trait object. Ran a few tests and this seems to partially work. Will draft a PR shortly.

Emmanuel-Melon added a commit to Emmanuel-Melon/syncstorage-rs that referenced this issue Apr 12, 2020
Partial Fix. Convert extractors from Futures style to async await. This implementation utilizes trait objects as return values from the refactored functions because Rust doesn't support `async fn` in traits.

Issue mozilla-services#544
@fzzzy
Copy link
Contributor Author

fzzzy commented Apr 15, 2020

Thanks. I looked at b15cf0d and I don't think we want to use the dyn Future approach. Did you try using Async Trait? That is what we are planning on moving to in the whole codebase eventually.

In the meantime, you can find other places in the codebase where we work around this. We do it by using a non-async function in the impl, but then using an async closure inside that to be able to use async syntax. However, I think this can also be replaced with Async Trait.

@Emmanuel-Melon
Copy link
Contributor

Thanks. I looked at b15cf0d and I don't think we want to use the dyn Future approach. Did you try using Async Trait? That is what we are planning on moving to in the whole codebase eventually.

In the meantime, you can find other places in the codebase where we work around this. We do it by using a non-async function in the impl, but then using an async closure inside that to be able to use async syntax. However, I think this can also be replaced with Async Trait.

Yes, I've suggested using async trait in the previous comment and I wanted a confirmation on whether I should introduce new dependencies.

So, can I go back to writing these functions using async trait?

@fzzzy
Copy link
Contributor Author

fzzzy commented Apr 16, 2020

Yes, please do. Hopefully it works easily. If not, discuss it in the channel.

@ethowitz
Copy link
Contributor

JIRA

@ethowitz
Copy link
Contributor

I could be wrong about this, but since the FromRequest trait isn't defined with #[async_trait], I don't think we can apply #[async_trait] to a FromRequest implementation, so I'm not sure if this is possible

@pjenvey
Copy link
Member

pjenvey commented Mar 14, 2022

AFAIK you're right: it's not possible to use async-trait for FromRequest impls

@ethowitz
Copy link
Contributor

There are certain places where we store a fut local var and call and_then instead of wrapping everything in an async {} block and using async/await, so I'll repurpose this issue to address those extractors

@pjenvey
Copy link
Member

pjenvey commented Jun 27, 2024

This was already completed with a few different PRs, closing

@pjenvey pjenvey closed this as completed Jun 27, 2024
Services Engineering automation moved this from Backlog: Sync to Done Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 Estimate - l - Moderately complex, will require some effort but clearly defined. good first bug Low priority, but valuable contributions sync-modernization
Projects
Development

Successfully merging a pull request may close this issue.

4 participants