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

Tracking Issue for Async I/O migration #1065

Open
jebrosen opened this issue Aug 2, 2019 · 28 comments
Projects

Comments

@jebrosen
Copy link
Collaborator

@jebrosen jebrosen commented Aug 2, 2019

Rocket will be asynchronous both internally and externally with respect to I/O. The public API will be based on std::future::Future and rust's async/await feature (see rust-lang/rust#50547) along with other traits in futures 0.3 such as AsyncRead. Progress and current problems with the implementation will be tracked in this issue.

If you want to try it out, look to the async branch. Expect some churn, unimplemented features, and breakage especially around async-aware traits such as FromData and Responder. However, many routes should continue to work with only minor modifications as long as any data guards and responders they use have been updated.

We welcome any feedback on the changes and on open design questions. If you want to help by migrating some code to async see "Next Steps", many of which are "help welcome/wanted". Pull requests to implement any functionality or fix any bug listed in this issue should be made against the async branch. If you are implementing something that will take some time, please leave a note so efforts are not duplicated too much.

Design

  • High-level APIs (routes and catchers):
    • Routes can now be defined as #[get|post|route(...)] async fn(params...) -> R where R is any Responder type. Routes that do not await anything can remain unchanged.
      • The same applies to #[catch]
    • Methods on Data now return Futures that must be .awaited. As a consequence, any route that uses Data directly must be async. Routes that use data guards such as Json are not affected by this change.
    • Routes that make their own HTTP requests or perform I/O should be async, as it will allow other requests to continue while waiting for the operation.
  • Mid and low-level APIs (FromData and FromDataSimple, on_response, Responder)
    • Data is read asynchronously: DataStream implements AsyncRead instead of Read. Thus, FromData::transform and FromData/FromDataSimple::from_data now return futures.
    • Response bodies are streamed to the client asynchronously: set_body and related methods expect an AsyncRead instead of Read.
      • Consequently, Fairing::on_response and Responder::respond_to now return futures. This allows responders and fairings to access and/or replace the body data.
  • Testing APIs (LocalRequest and LocalResponse)
    • LocalResponse: body_bytes_wait and body_string_wait are introduced as shorthand for futures::executor::block_on(self.body_{bytes|string}()) so that tests do not need to create an executor themselves.
  • Typed headers are gone, but we might bring them back
  • Many long-standing bugs with keep-alive, graceful shutdown (#180), performance, and more are fixed or will be easier to fix.
  • Some new functionality will be possible to implement, such as SSE and WebSockets. Many of these will not necessarily be implemented as part of this particular effort, however.

Open Questions / TODO

  • The handler, fairing, and responder APIs. These will likely have to change slightly, especially the lifetime parameters and bounds, based on work on other tasks if we discover use cases that can't be satisfied by the current API.
    • In fn respond_to(self, req: &'r Request<'_>) -> response::ResultFuture<'r>, the Future and the Request are constrained to the same lifetime. I think something like fn respond_to(self, req: &'a Request<'_>) -> response::ResultFuture<'a + 'r> would probably be better, but I haven't come up with a concrete example of when you might want that and it's also invalid syntax. The correct syntax for expressing this is unwieldy and appears to be unusable in HRTBs.
    • dtolnay/async-trait would be really helpful for this, but is blocked by dtolnay/async-trait#34 (rust-lang/rust#60658).
  • Currently, routes must either be fn -> R where R: Responder (non-async) or async fn -> R where R: Responder (async). Should we try to support (non-async) fn -> impl Future<Output=R> where R: Responder?
  • Investigate and/or decide what happens when panics happen inside routes and/or things they are .awaiting. The current behavior looks similar to the 0.4 behavior but hasn't been looked at closely.
  • #1066 - Add a Write-like interface for responses
  • #1067 - Remove or replace typed headers
  • #1068 - Allow running Rocket on a custom executor - #1097
  • #180 - Graceful shutdown. The main design questions are how/where to add the APIs for "do shutdown now" and how to support servers that "can't" shut down. - #1079
  • #1071 - Use the futures 0.3 versions of hyper and/or tokio.
  • #1117 - Asynchronous database pools, or at the very least forward compatibility with them.
  • Design a publicly usable async test entry point or macro, or ensure that tokio 0.2's #[tokio::test] works properly - there is now a #[rocket::async_test] macro, subject to change
  • Adding an asynchronous version of FromRequest, either in addition to or instead of a synchronous FromRequest.
  • Making more fairing methods asynchronous, such as on_attach or on_launch.
  • Keep-alive and idle client timeouts
  • Handling of certain existing synchronous or blocking code, such as databases. If not done carefully, executor deadlocks can occur.

Current status

  • Complete: First round of changes to APIs in http , core, and codegen to enable async/await all the way from request to response: @jebrosen and @schrieveslaach, #1008
  • Complete: most of rocket_contrib. rocket_contrib::compression is not complete yet.
    • databases - Done, but still based on synchronous r2d2. We are not necessarily pushing for asynchronous database engines, but we should see if they will be usable. postgres is a good candidate for exploring this.
  • Complete: many examples and corresponding tests. ~~ Tests that use File (or serve) are using an unstable async_test function which probably won't be resolved until at least #1071. ~~
  • Complete: Nearly all examples and tests, aside from rocket_contrib::compression.
  • Missing: typed headers (#1067)
  • Complete: TLS (#1070)

Next steps

Waiting on / upstream blockers

Implementation

  • rocket_contrib::compression - in progress (@jebrosen)

Documentation

  • Update documentation for all affected APIs.
  • Update the guide.
  • We should consider linking to a resource such as the rust async book as a general-purpose async primer.

Clean ups and fixes

  • Pin<Box<dyn Future<Output=Something<'r>> + Send + 'r>> is both unwieldy and used frequently. We should create and employ trait aliases where possible. Some of these exist already, e.g. FromDataFuture, but we might want to rename or split them up more.
    • As of #1125 we have aliases such as FromDataFuture of futures::future::BoxFuture, itself an alias for Pin<Box<Future<Output=T> + Send + 'a>>.
  • Miscellaneous TODOs (tagged as TODO.async) throughout code.
  • There are probably places where a Pin<Box<Future<...>>> is returned when an impl Future<...> or async fn could be used instead. This is more likely on internal functions than public ones.
  • Any remaining "get this working quick" hacks; general polish. Try not to make too much work in this area.
    • Now that rust-lang-nursery/futures-rs#1788 has been merged, we should replace our Take adapter with that one once it's in a released version of futures-preview.
    • Another example is the way response bodies are passed back to hyper. There are a few inefficient channels currently in use that copy data around more times than should be necessary.
@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 2, 2019

The async_await feature is now required in addition to proc_macro_hygine and decl_macro

Wasn't decl_macro removed on the master branch a while back?

Currently, routes must either be fn -> R where R: Responder (non-async) or async fn -> R where R: Responder (async). Should we try to support (non-async) fn -> impl Future<Output=R> where R: Responder?

I don't see why not. Though it would be relatively trivial to create it async and await the returned value(s).

I'll take a look at uuid tomorrow to see if any changes are necessary. I suspect the answer is no.

@jebrosen Roughly how much of the documentation can be switched over to async? Are we going to primarily document in async, or will we essentially double everything up?

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Aug 2, 2019

Wasn't decl_macro removed on the master branch a while back?

Right! I'll fix that.

Currently, routes must either be fn -> R where R: Responder (non-async) or async fn -> R where R: Responder (async). Should we try to support (non-async) fn -> impl Future<Output=R> where R: Responder?

I don't see why not. Though it would be relatively trivial to create it async and await the returned value(s).

The issue is the return type of routes. Currently the codegen assumes that a non-async function returns some type R that implements Responder and an async function returns a Future whose output type is some R implementing Responder. Allowing functions to return either a Responder or a Future is hairy because both are traits and pretty soon we would end up wrestling with specialization. But I might have missed something in my analysis, and I would really like to support it if we can!

@jebrosen Roughly how much of the documentation can be switched over to async? Are we going to primarily document in async, or will we essentially double everything up?

The only place where there is a choice is routes; currently everything else is an async-only API and documentation should reflect that. I was going to wait a bit before pushing hard for docs, as there are likely a few design issues to be resolved with which APIs return which futures with what lifetime bounds.

My admittedly hand-wavy assumption was that we would explain traits like FromData and Responder in terms of async and futures, but use mostly non-async routes. We should definitely have a few examples of async routes, perhaps for HTTP requests or File I/O. For a lot of routes I do expect it to be as easy as changing fn to async fn and using .await in the right places.

Another really important point here is that manual FromData and Responder implementations should be relatively rare and are much more in the domain of API documentation than in the guide. I am personally more worried about the accessibility and understandability of the guide.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 2, 2019

The issue is the return type of routes. Currently the codegen assumes that a non-async function returns some type R that implements Responder and an async function returns a Future whose output type is some R implementing Responder. Allowing functions to return either a Responder or a Future is hairy because both are traits and pretty soon we would end up wrestling with specialization. But I might have missed something in my analysis, and I would really like to support it if we can!

That certainly makes sense. Adding support for -> impl Future would be backwards compatible, as it doesn't currently work. So that's a point that can certainly be postponed to see if there's even demand after async lands.

The only place where there is a choice is routes; currently everything else is an async-only API and documentation should reflect that. I was going to wait a bit before pushing hard for docs, as there are likely a few design issues to be resolved with which APIs return which futures with what lifetime bounds.

Probably best to hold off on docs for the time being, then.

@Nemo157

This comment has been minimized.

Copy link

@Nemo157 Nemo157 commented Aug 2, 2019

compression - Not for the faint of heart; there might be some important APIs that are not yet available. HELP WELCOME

Quick advertisement for https://github.com/rustasync/async-compression, this should allow just drop-in wrappers to transparently compress/decompress AsyncRead/AsyncWrite instances. Not all encodings are supported for the read/write interfaces yet as the first framework it was used in used the impl Stream<Item = io::Result<Bytes>> adaptors instead, but it should be straightforward to expand them (at least once I merge the initial AsyncWrite support).

@theduke

This comment has been minimized.

Copy link

@theduke theduke commented Aug 2, 2019

@jebrosen this is awesome!

Just as a side note, both tokio and hyper master are working with std::future::Future now. It's still a WIP but in my testing the essentials are all working, so this could already be based on the master branches instead of dealing with the compat layer.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 2, 2019

contrib/lib/src/uuid.rs looks good to me as-is. It's just a thin wrapper around the uuid crate, so there's not really any room for making things async, unless we were to go all in and parse it asynchronously.

@alexsherman

This comment has been minimized.

Copy link

@alexsherman alexsherman commented Aug 5, 2019

databases - We are not necessarily pushing for asynchronous database engines, but we should at least ensure they will be usable. postgres is a good candidate for exploring this.

Hi - what would you be looking for in a pull request to explore this? I've been developing some projects using Rocket and have been using both tokio-postgres and the synchronous rust-postgres, as well as their connection pools, and I would like to help if I think I know how.

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Aug 6, 2019

Hi - what would you be looking for in a pull request to explore this?

I have not tried too hard to test what "ensure they will be usable" entails. There are a few problems I can see ahead of time:

  • Futures-aware database engines probably need to be run within a specific runtime (i.e. tokio 0.1 or tokio 0.2). So once Rocket uses tokio 0.2 (#1071), you wouldn't be able to use the current version of tokio-postgres. This is a non-obvious compatibility problem that isn't expressed in any type signature.
  • "Connect" might be an asynchronous operation. This seems to be the case for tokio-postgres, for example. So we should evaluate if making FromRequest return a future would be worth it for making databases (among other things) easier to work with.

At a minimum, I think we should be able to:

  • Create a connection pool with tokio-postgres and/or bb8 and put it in managed state. This should tell us if on_attach and on_launch need to be async-capable.
  • Create a request guard type wrapping a connection (or a future that will resolve to a connection). This and the previous bullet point together are more or less what the guide demonstrated for databases in Rocket 0.3 before we had rocket_contrib::databases.
  • Use said connection via the request guard within a route to query/alter the database. This should work fine, because routes can now be async fn.
  • Test, figure out, and document cross-runtime compatibility (or lack thereof). That is, a definitive statement in the form of "Rocket uses tokio 0.2, so your database futures have to as well" if that's the case. Alternatively, maybe we could find a proof of concept of running two runtimes - tokio 0.2 (for rocket) alongside tokio 0.1 (for tokio-postgres) with some kind of message passing between them. I leave this intentionally vague because it's an annoying solution to an annoying problem and someone other than I will probably think of a better idea.
@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Aug 18, 2019

I am mostly waiting on #1071 (after a new tokio alpha and hyperium/hyper#1897) so that we don't need to significantly rewrite too much code that directly deals with the low-level bits -- which are pretty different between tokio 0.1 and 0.2. For example, I suspect #1070 will take almost as much work again as it would to implement it now.

At present all public APIs use async/await and futures 0.3, but everything runs on a tokio 0.1 runtime. This means that I/O types from tokio 0.2 cannot be used directly in routes yet, for example.


For anyone interested in kicking the tires pretty early on, I have written a short and possibly incomplete guide to trying projects against the async branch. At this stage the most helpful feedback would be examples of "things I can't do anymore" or that now require expensive allocations, especially in implementations of FromData or Fairing.

  • Depend on rocket = { git = "https://github.com/SergioBenitez/Rocket", branch = "async" } and rocket_contrib = { git = "https://github.com/SergioBenitez/Rocket", branch = "async" }
  • Add #![feature(async_await)] to your crate. You can remove #[feature(decl_macro)] thanks to #964.
  • Typed headers are removed and a replacement has not been decided on (#1067), so you will need to build headers manually and/or use raw headers at least for now
  • Implementations of FromData, Fairing, AdHoc::on_response, and Responder must be converted to use futures. For some cases, wrapping the function body in Box::pin(async move { }) will be enough. For some traits, just like with FromData for rocket_contrib::json::Json, you might need to copy some data out of the request before creating the future.
  • Some dependencies such as ring and handlebars have been updated, which may consequently break your application. You should get obvious build failures if this affects you.
  • You can now declare routes as async, and await futures inside of them.
@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 18, 2019

@jebrosen have you taken a look at async-trait? I know I've mentioned it before, and it reduces some of the overhead associated with async trait fns, like lifetimes and boxing. As a bonus, all we'd have to do after GATs are (some day) implemented is remove the attribute.

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Aug 18, 2019

I did try async-trait today and ran into some issues that might be able to be worked around. I would prefer to use async-trait on either all affected traits or none of them, and I'm hopeful that it will be usable.

@Nemo157

This comment has been minimized.

Copy link

@Nemo157 Nemo157 commented Aug 19, 2019

As a bonus, all we'd have to do after GATs are (some day) implemented is remove the attribute.

Unfortunately that would be a breaking change to any public traits, as far as I know there's no way to be forward compatible with GATs in traits. You can be forward compatible with using 'static bound async blocks in traits (once type_alias_impl_trait is stabilized) by using an associated type and setting that to be the pinned-box in implementations, e.g. skimming the changes it looks like FromDataSimple could be defined like this (and maybe FromData since it uses an external lifetime), but Fairing can't as it borrows from its arguments.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 19, 2019

Yeah, it would definitely be a breaking change, as APIs would be different. I was referring to the changes in the code itself, which would amount to deleting a few lines.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 19, 2019

Update on the attempt to use async-trait everywhere: it's currently not possible. There is a bug in the compiler that prevents async default fn or default async fn, which is necessary where specialization is currently used.

@nicoburns

This comment has been minimized.

Copy link

@nicoburns nicoburns commented Aug 21, 2019

Test, figure out, and document cross-runtime compatibility (or lack thereof). That is, a definitive statement in the form of "Rocket uses tokio 0.2, so your database futures have to as well" if that's the case. Alternatively, maybe we could find a proof of concept of running two runtimes - tokio 0.2 (for rocket) alongside tokio 0.1 (for tokio-postgres) with some kind of message passing between them. I leave this intentionally vague because it's an annoying solution to an annoying problem and someone other than I will probably think of a better idea.

tokio-postgres has a std-futures branch with an in-progress port to tokio 0.2 (and std::future). We could probably just specify that that version is needed for compatibility.

@Centril

This comment has been minimized.

Copy link

@Centril Centril commented Aug 21, 2019

Update on the attempt to use async-trait everywhere: it's currently not possible. There is a bug in the compiler that prevents async default fn or default async fn, which is necessary where specialization is currently used.

I fixed this bug on nightly. Please make sure however that your uses of default async? fn are #[cfg(...)] gated in a separate module once you decide to make rocket work on stable.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 21, 2019

Thanks @Centril! I believe @jebrosen has some way to avoid needing specialization — I recall seeing it being eliminated in a PR or something like that.

Hopefully others can now use async-trait in more situations.

@jhpratt

This comment has been minimized.

Copy link
Contributor

@jhpratt jhpratt commented Aug 28, 2019

After attempting to convert r-spacex/Titan to the current HEAD of the async branch (dcea956), I ran into a glaring issue that needs to be addressed before even a prerelease. Currently, diesel (at least its PostgreSQL type) doesn't implement Sync, so it can't be used for a large swath of cases. @jebrosen has pointed out that it does implement Send, so it can be used in some cases.

While not directly related to Rocket, this is something we'll likely want to work with @sgrif et al. to ensure full compatibility.

@omac777

This comment has been minimized.

Copy link

@omac777 omac777 commented Sep 3, 2019

I tried the async hello world it was working fine.
When I tried to convert it into a tls version so it resembles the tls example from the non-async rocket master, it was still serving up a http url. I confirmed this to be a true bug because if I just switch to the master branch within the Cargo.toml, it does enable tls and does serve up a https url.

[dependencies]
rocket = { git = "https://github.com/SergioBenitez/Rocket", features = ["tls"], branch = "async" }
# rocket = { git = "https://github.com/SergioBenitez/Rocket", features = ["tls"] }

THIS IS THE ASYNC BRANCH OUTPUT:
 $ ROCKET_ENV=dev ./target/release/helloasyncrocket 
🔧 Configured for development.
    => address: localhost
    => port: 8000
    => log: normal
    => workers: 24
    => secret key: generated
    => limits: forms = 32KiB
    => keep-alive: 5s
    => tls: enabled
🛰  Mounting /:
    => GET / (hello)
🚀 Rocket has launched from http://localhost:8000


I was expecting to see an output similar to the one from THE NON-ASYNC BRANCH OUTPUT behaving correctly:
$ ROCKET_ENV=dev ./target/release/helloasyncrocket 
🔧 Configured for development.
    => address: localhost
    => port: 8000
    => log: normal
    => workers: 24
    => secret key: generated
    => limits: forms = 32KiB
    => keep-alive: 5s
    => tls: enabled
🛰  Mounting /:
    => GET / (hello)
🚀 Rocket has launched from https://localhost:8000
@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Sep 3, 2019

Yes, TLS is not implemented yet. I'll make that clearer in the main comment.

@Follpvosten

This comment has been minimized.

Copy link

@Follpvosten Follpvosten commented Oct 24, 2019

Not sure if this is the right place for such a question, but here we go:

I'm currently writing a backend for a web app with Rocket, using tokio-postgres directly at the moment (but this should apply to pools as well).
To remove boilerplate at the start of each handler which needs database access, I've implemented FromRequestAsync for my typed wrapper around the connection, so I can just fn handler(client: db::Client) -> ...
Since connecting to a database is an inherently expensive operation, this is an obvious use-case for request-local state; however, both establishing a connection with an async driver and retrieving one from an async pool are async operations, and local_cache currently doesn't have an async equivalent.

Would it be possible to implement such an async variant? I have no idea how much work would have to go into that, as far as I can tell, it probably wouldn't be as elegant as the current local_cache implementation, but it should be doable (taking a future instead of a closure and awaiting it once). If it's not too hard, maybe I could even contribute it.

EDIT, 10 minutes later:

pub async fn local_cache_async<T, F>(&self, fut: F) -> &T
    where F: std::future::Future<Output = T>,
            T: Send + Sync + 'static
{
    if let Some(s) = self.state.cache.try_get() {
        s
    } else {
        self.state.cache.set(fut.await);
        self.state.cache.get()
    }
}

I just want to say that I love async.

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Oct 24, 2019

Why use local_cache for this at all instead of a connection pool? Do you need to use the same connection in many request guards and fairings while processing one route?

@Follpvosten

This comment has been minimized.

Copy link

@Follpvosten Follpvosten commented Oct 24, 2019

You're right, it's not a good example; I'm currently waiting for Rocket to implement async pooling support, that's why I use the connection directly.

However, as I said, this also applies to async pools, and if I want to implement something like FromRequestAsync for User and Admin with an async connection pool (to go with the example in the docs), I'll also want to query the database async. That should be the reason for providing an async local cache variant, not my specific example.

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Oct 24, 2019

You can still implement FromRequestAsync for User with a connection pool, without the local cache (e.g. https://git.jebrosen.com/jeb/sirus/src/branch/async-db-pool-icky/sirus-server/src/guards.rs#L39). The local cache would save the (hopefully small!) expense of pulling from the pool multiple times, though, so it's still a good idea - I just don't see why it's necessary.

(side note: let's move this conversation to #1117)

@Follpvosten

This comment has been minimized.

Copy link

@Follpvosten Follpvosten commented Oct 25, 2019

Sorry if this was unclear, but as far as I can see, the expensive blocking operation in this example would be pulling the User from the database, and caching it would avoid doing everything twice.

This example for request-local state is what I'm talking about - avoiding multiple connections and importantly, multiple queries as well (like db.get_user(id)), which I think should also be async when using an async database driver. (Your website seems to pull the User in a blocking way and each time the request guard gets called)

So the async version of the second example would look something like this (imo, haven't checked if it would even compile):

impl<'a, 'r> FromRequestAsync<'a, 'r> for &'a User {
    type Error = !;

    fn from_request<'fut>(request: &'a Request<'r>) -> FromRequestFuture<'fut, &'a User, !>
        where 'a: 'fut
    {
        Box::pin(async move {
            // This future will run at most once per request, regardless of
            // the number of times the `User` guard is executed.
            let user_result = request.local_cache_async(async {
                let db = try_outcome!(Database::from_request(request).await.succeeded());
                if let Some(id) = request.cookies()
                    .get_private("user_id")
                    .and_then(|cookie| cookie.value().parse().ok()) {
                    // The important part
                    db.get_user(id).await.ok()
                } else {
                    None
                }
            }).await;

            user_result.as_ref().or_forward(())
        })
    }
}

impl<'a, 'r> FromRequestAsync<'a, 'r> for Admin<'a> {
    type Error = !;

    fn from_request<'fut>(request: &'a Request<'r>) -> FromRequestFuture<'fut, Admin<'a>, !>
        where 'a: 'fut
    {
        Box::pin(async move {
            let user: &User = try_outcome!(User::from_request(request).await);

            if user.is_admin {
                Outcome::Success(Admin { user })
            } else {
                Outcome::Forward(())
            }
        })
    }
}

Or am I getting something wrong, did something change since that example was written, so it's not a valid concern anymore?
I'm okay with moving this to #1117 if it's not relevant to async Rocket in general.

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Oct 25, 2019

as far as I can see, the expensive blocking operation in this example would be pulling the User from the database

Ah, I wasn't even thinking of the case where the request guard is actually used multiple times. I usually try to structure routes in such a way that those things don't happen too often.

I've added local_cache to the list of APIs that need to be async or have async equivalents.

@Follpvosten

This comment has been minimized.

Copy link

@Follpvosten Follpvosten commented Oct 25, 2019

I usually try to structure routes in such a way that those things don't happen too often.

That was my first thought as well - I noticed this because I had a "/category/<id>" route and a "/category/<name>", rank = 2 route, and the second one took twice as long to execute. My first thought was to change the routes to something like "/category/by-name/" and "/category/by-id/", but that felt like a workaround since I had already seen the local cache example in the FromRequest docs, so I tried to apply that. (Probably wouldn't even have noticed if I had been using a connection pool.)

I've added local_cache to the list of APIs that need to be async or have async equivalents.

Since I already have a working implementation, should I open a PR adding it? (I currently call it local_cache_async)

@jebrosen

This comment has been minimized.

Copy link
Collaborator Author

@jebrosen jebrosen commented Oct 26, 2019

Since I already have a working implementation, should I open a PR adding it?

Sure! We can also work out whether it's valuable to keep both or perhaps only the async version in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Async I/O
  
In progress
9 participants
You can’t perform that action at this time.