Async: on_attach fairings #1242

jebrosen · 2020-02-26T02:11:07Z

This PR makes on_attach fairings asynchronous. It transitively requires several other functions to become asynchronous, including the "entry point" (usually launch() or Client::new()). This specific implementation is a balance between needing to do asynchronous work in on_attach, such as database setup, versus the verbosity of making too many builder-type functions require .awaits.

Some other similarly sweeping changes that I have not made yet, and can be added to this or made as a followup PR:

A synchronous version of Client (with an internal Runtime, that would block_on each operation).
- This can simplify many tests by making .awaits internal - especially in e.g. https://github.com/SergioBenitez/Rocket/compare/async...jebrosen:async-on-attach?expand=1#diff-048d6496580899aa2aec5fc6a244faf0
Changing the return type of launch() so that the most common usage does not require let _ = to suppress an unused Result warning.

Some other things we may want, off the top of my head:

An explanation of why #[rocket::main] and #[rocket::async_test] exist (short version: an attribute is worth it, and reexports of tokio::main and tokio::test don't work)

SergioBenitez · 2020-03-03T00:34:21Z

This is really excellent work, @jebrosen! Please see my inline comments. The two biggest suggestions are:

Renaming RocketInner to Manifest, exposing Manifest directly instead of wrapping it in an Inspector.
Changing the signature of on_attach to allow returning unboxed futures.

core/lib/src/fairing/ad_hoc.rs

contrib/codegen/src/database.rs

contrib/codegen/tests/ui-fail/database-types.rs

contrib/lib/tests/templates.rs

core/codegen/src/attribute/mod.rs

core/lib/src/rocket.rs

SergioBenitez · 2020-03-03T00:20:15Z

core/lib/src/rocket.rs

        use std::net::ToSocketAddrs;

        use crate::error::Error::Launch;

-        let full_addr = format!("{}:{}", self.config.address, self.config.port);
+        self.finish().await;
+


Lots of vertical space here...

core/lib/src/rocket.rs

SergioBenitez · 2020-03-03T00:28:15Z

core/lib/src/rocket.rs

    ///     assert!(result.is_ok());
    /// # }
    /// }
    /// ```
-    pub async fn serve(self) -> Result<(), crate::error::Error> {
+    pub async fn launch(mut self) -> Result<(), crate::error::Error> {


I know most the contents of this function were introduced in some other commits, but it has become very difficult to follow. The combination of macros and cfgs, in particular, really help to obfuscate the functionality. We really must improve this before we release 0.5.

scripts/test.sh

….toml. This is not supported and is the same as putting the contents in [dependencies] anyway. It became a warning in rust-lang/cargo#7660.

observed. This is a prerequisite for async on_attach fairings. `Rocket` is now a builder wrapper around the private type `RocketInner`, with operations being run the next time it is necessary: `launch()`, `Client::new()`, or `inspect()`. `inspect()` returns an `Inspector<'_>`, which is analogous to and has the methods that could be called on an `&Rocket`.

jebrosen · 2020-03-06T20:02:54Z

I've added commits resolving several comments, but I am not quite finished yet / have a few follow-up questions.

core/lib/src/rocket.rs

core/lib/src/local/client.rs

…lled afterwards anyway.

This transitively requires that `Rocket::inspect`, `Client::new`, and `Client::untracked` also become async.

…m the caller.

…ice panics.

`#[rocket::main]` works like `#[rocket::async_test]`, but it uses tokio's multithreaded scheduler.

…ions to a Kind type.

jebrosen · 2020-03-10T15:04:15Z

This is ready for another review.

SergioBenitez

No major requests this time around. Excellent work!

A few notes:

Is it possible to have async fn state() and async fn config() on Rocket itself? It would be nice not to have to chain through manifest() when possible.
In general, many functions now take a Manifest, and I now wonder: should they take a Rocket (or &mut Rocket, or whatever it need be) and internal retrieve the manifest as necessary? This would make the functions async, but in some cases (say DbConn::get_one(rocket).await), this API seems cleaner and more logical. What do you think?
I recall us discussing allowing the minimal Rocket application to look like:
```
#[rocket::main]
async fn main() {
    rocket::ignite().mount("/", routes![index]).launch()
}
```
Perhaps without the async on fn, but the idea being you don't need to await the return value of launch() in the common case. What is the status of that?

core/lib/src/fairing/ad_hoc.rs

SergioBenitez · 2020-06-04T04:53:38Z

core/lib/src/fairing/ad_hoc.rs

-            let mut opt = mutex.lock().expect("AdHoc::Attach lock");
-            let f = opt.take().expect("internal error: `on_attach` one-call invariant broken");
-            f(rocket)
+            let f = mutex


Perhaps we can simply move the .lock() to the same line as mutex.

SergioBenitez · 2020-06-04T04:56:24Z

core/lib/src/request/state.rs

-/// let state = State::from(&rocket).expect("managing `MyManagedState`");
+/// # rocket::async_test(async {
+/// let mut rocket = rocket::ignite().manage(MyManagedState(127));
+/// let state = State::from(rocket.inspect().await).expect("managing `MyManagedState`");


Don't we have a method on Rocket to get State directly? I imagine it would be cleaner to use that here.

I was trying to avoid making any unrelated changes, but I agree it seems redundant. Commit 80e7339 introduced Rocket::state in 2017, and commit 983ee9b introduced State::from in 2018.

That's fair. I think the idea here was to use that the two were equivalent. The addition of .inspect() and .await makes this feel overly verbose, however.

core/lib/src/rocket.rs

SergioBenitez · 2020-06-04T05:30:01Z

site/guide/3-overview.md

-  `Rocket::spawn()`.
+  Rocket 0.5 uses the tokio (0.2) runtime. The runtime is started for you
+  if you use `#[rocket::main]`, but you can still `launch()` a rocket
+  instance on a custom-built `Runtime`.


This requires some pointers on how to do so. Is that material available?

Not necessarily all in one place - meaningfully using a custom-built Runtime means using Builder and block_on, which are documented separately in the API. Something like this would be such an example:

let runtime = tokio::runtime::Builder::new() .basic_scheduler() // use the single-threaded runtime, for illustration purposes .build() .unwrap(); runtime.block_on(rocket::ignite().mount("/hello", routes![world]).launch());

We should perhaps open up an issue, if one doesn't already exist, to document all of the things that need to be documented before we release async.

jebrosen

Is it possible to have async fn state() and async fn config() on Rocket itself? It would be nice not to have to chain through manifest() when possible.

I think so; I will try to add those.

In general, many functions now take a Manifest, and I now wonder: should they take a Rocket (or &mut Rocket, or whatever it need be) and internal retrieve the manifest as necessary? This would make the functions async, but in some cases (say DbConn::get_one(rocket).await), this API seems cleaner and more logical. What do you think?

At a quick glance, DbConn::get_one looks like the only place that happens (were there others?) and it should be possible to do so. In general though, &mut is a bit annoying since it can make it more difficult to get all of config, managed state, and a database connection simultaneously - that's why Manifest takes &self.

I recall us discussing allowing the minimal Rocket application to look like (thing without a .await)

I think this risks too much confusion. These are what I'm aiming for here:

Have a single function to document and explain - i.e. no split between async fn serve() and fn launch().
Have it be async, to work better next to other async code (like setting up another server) and for better composability like select!(rocket.launch(), timeout) or join!(rocket.launch(), websocket_server.run())
Be consistent with other functions that return futures, and with usage inside an async fn that is not main or decorated with an attribute. I think it would be confusing to need to .await or not .await launch() for different reasons than for other futures.

core/lib/src/fairing/ad_hoc.rs

jebrosen · 2020-06-05T16:35:15Z

core/lib/src/request/state.rs

-/// let state = State::from(&rocket).expect("managing `MyManagedState`");
+/// # rocket::async_test(async {
+/// let mut rocket = rocket::ignite().manage(MyManagedState(127));
+/// let state = State::from(rocket.inspect().await).expect("managing `MyManagedState`");


I was trying to avoid making any unrelated changes, but I agree it seems redundant. Commit 80e7339 introduced Rocket::state in 2017, and commit 983ee9b introduced State::from in 2018.

jebrosen · 2020-06-05T16:46:08Z

core/lib/src/rocket.rs

-        impl<Fut> hyper::Executor<Fut> for TokioExecutor where Fut: Future + Send + 'static, Fut::Output: Send {
-            fn execute(&self, fut: Fut) {
-                tokio::spawn(fut);
+    pub(crate) fn finish(&mut self) -> BoxFuture<'_, ()> {


The recursion was already there, since on_attach fairings can themselves call attach - it has just moved. Switching to an async fn / Future changes this to a recursion in the type system instead of in the call stack, which is why Boxing is a solution.

As for a name change, I think I don't like process because it's undescriptive and process_manifest sounds like the manifest is the input - maybe one of process_pending, apply_pending, prepare_manifest, update_manifest?

jebrosen · 2020-06-05T16:57:04Z

site/guide/3-overview.md

-  `Rocket::spawn()`.
+  Rocket 0.5 uses the tokio (0.2) runtime. The runtime is started for you
+  if you use `#[rocket::main]`, but you can still `launch()` a rocket
+  instance on a custom-built `Runtime`.


Not necessarily all in one place - meaningfully using a custom-built Runtime means using Builder and block_on, which are documented separately in the API. Something like this would be such an example:

let runtime = tokio::runtime::Builder::new() .basic_scheduler() // use the single-threaded runtime, for illustration purposes .build() .unwrap(); runtime.block_on(rocket::ignite().mount("/hello", routes![world]).launch());

jebrosen · 2020-06-05T17:43:23Z

core/lib/src/rocket.rs

-        }
-
+    pub fn manage<T: Send + Sync + 'static>(mut self, state: T) -> Self {
+        self.pending.push(BuildOperation::Manage(Box::new(|rocket| {


I didn't do anything yet. Here's where it stands:

mount panics immediately if base is invalid. It will raise a panic in finish if a route URI is found to be invalid.

manage panics in finish if it's duplicated state.

attach says it's immediate, but it actually happens in finish.

I think we could change mount to panic immediately, if we're okay reordering some log output.

Do you have any ideas for rewording the documentation? I find this a bit awkward: "If the fairing is an attach fairing, it will be attached when necessary (e.g. when calling inspect or launch)"

core/lib/src/rocket.rs

core/lib/src/fairing/ad_hoc.rs

…nightly. This partially (re-)applies f4bb8bb

…nience.

SergioBenitez · 2020-06-06T07:29:34Z

I recall us discussing allowing the minimal Rocket application to look like (thing without a .await)

I think this risks too much confusion. These are what I'm aiming for here:

Have a single function to document and explain - i.e. no split between async fn serve() and fn launch().

Have it be async, to work better next to other async code (like setting up another server) and for better composability like select!(rocket.launch(), timeout) or join!(rocket.launch(), websocket_server.run())

Be consistent with other functions that return futures, and with usage inside an async fn that is not main or decorated with an attribute. I think it would be confusing to need to .await or not .await launch() for different reasons than for other futures.

Your last point actually convinces me that we should allow a non-.launch().await version if and only if the main function is declared as non-async. This is consistent with routes which, when not declared async, must not return a future. If your main isn't doing anything async, there's no reason why you should need to async fn it.

The challenge, of course, is that launch() needs to have one type signature: I don't think we should employ any magic here. Perhaps the solution is to ask main to return a value and have Rocket do the appropriate thing with it. Perhaps we allow returning a Rocket and a impl Future<Output=Rocket> (or something similar)?

SergioBenitez

Awesome. Just need to get the tests passing now.

SergioBenitez · 2020-06-06T07:32:33Z

core/lib/src/catcher.rs

@@ -153,7 +153,7 @@ macro_rules! default_catchers {
        let mut map = HashMap::new();

        $(
-            fn $fn_name<'r>(req: &'r Request<'_>) -> futures_util::future::BoxFuture<'r, response::Result<'r>> {
+            fn $fn_name<'r>(req: &'r Request<'_>) -> futures::future::BoxFuture<'r, response::Result<'r>> {


Should this be $crate::futures::, to be safe?

Since this macro isn't usable in any other module(s), the only conflict would be with another valid path futures::future::BoxFuture reachable from mod defaults below. IMO $crate will look verbose/out of place here with no benefit.

SergioBenitez · 2020-06-06T07:38:03Z

core/lib/src/rocket.rs

-        impl<Fut> hyper::Executor<Fut> for TokioExecutor where Fut: Future + Send + 'static, Fut::Output: Send {
-            fn execute(&self, fut: Fut) {
-                tokio::spawn(fut);
+    pub(crate) fn finish(&mut self) -> BoxFuture<'_, ()> {


actualize_manifest, execute_manifest, consume_manifest?

jebrosen · 2020-06-07T04:21:29Z

The challenge, of course, is that launch() needs to have one type signature: I don't think we should employ any magic here. Perhaps the solution is to ask main to return a value and have Rocket do the appropriate thing with it. Perhaps we allow returning a Rocket and a impl Future<Output=Rocket> (or something similar)?

Is this set of examples in line with that proposal?

// Pulled out of main() for illustrative purposes.
fn rocket() -> Rocket {
    rocket::ignite()
        .attach(AdHoc::on_attach(|rocket| async {
            info!("Look! async!");
            Ok(rocket)
        }))
        .mount("/", rocket::routes![a, b, c])
}

// No setup helpers. launch() is an async fn, so it returns a Future that can be passed to block_on.
// Also notice the semicolon - this example assumes we fix launch() to return
// a type that is easily convertible to Result, is *not* must_use, and automatically logs
// an error if not inspected (like LaunchError in 0.4).
fn main() {
    create_a_runtime().block_on(rocket().launch());
}

// Using tokio::main
#[tokio::main]
async fn main() {
    async_setup().await;
    rocket().launch().await;
}
// This (roughly) expands to:
fn main() {
    create_runtime().block_on(async {
        async_setup().await;
        rocket().launch().await;
    })
}

// Using rocket::main with an async fn
#[rocket::main]
async fn main() -> Rocket {
    async_setup().await;
    rocket()
}

// Using rocket::main with a non-async fn.
#[rocket::main]
fn main() -> Rocket {
    rocket()
}

// These could expand to (roughly):
fn main() {
    create_runtime().block_on(async {
        [async] fn __inner_main() -> Rocket { /* original contents of main() */ }
        let rocket: ::rocket::Rocket = __inner_main()[.await];
        rocket.launch().await;
    })
}

The last example is the closest to what you described, and the desugaring is very similar to what is already done for routes to be either async fn or non-async fn. This API has these notable properties:

The #[rocket::main] style is extremely easy for getting started
There is no way to access the launch result programmatically - it relies entirely on the "print on drop" behavior
#[tokio::main] or custom-constructed Runtimes are also easy to use if necessary, by adding .launch().await.
- The future returned by launch() can also easily be used in a join! or select!
It does not satisfy "we should allow a non-.launch().await version if and only if the main function is declared as non-async.
- I did try, and was not super happy with, the version where changing fn main to async fn main additionally required adding .launch().await. IMO, that option would complicate both documentation and codegen.

jebrosen · 2020-06-07T21:09:11Z

I think I have addressed or replied to all comments at this point and this is ready for another review. I am finding GitHub's PR interface more difficult to navigate than I remember it, so I might have missed something.

SergioBenitez · 2020-06-13T00:31:36Z

Doing a final review now. I really like your proposal for a -> Rocket returning main.

There is no way to access the launch result programmatically - it relies entirely on the "print on drop" behavior

If the function doesn't return a value, I think we should have #[rocket::main] mean exactly #[tokio::main]. That way, a user need not import tokio, and they can still access the launch result programmatically, albeit by needing to add .launch().await, which they'd need to do anyway.

Edit: to be clear, we should do this in another ~~commit~~ pull request, not this one. I'm happy to take this on, in fact!

SergioBenitez

Some minor requests/questions, but I'm also okay merging this as is! Exciting!

SergioBenitez · 2020-06-13T00:48:58Z

contrib/codegen/src/database.rs

-            pub fn get_one(rocket: &::rocket::Rocket) -> Option<Self> {
-                rocket.state::<#pool_type>()
+            pub fn get_one(manifest: &::rocket::Manifest) -> Option<Self> {
+                manifest.state::<#pool_type>()


How did we feel about making this pub async fn get_one() and taking in a Rocket or &Rocket or &mut Rocket?

Actually, taking an &mut Rocket should be (mostly) fine:

let mut rocket = make_rocket(); let conn = rocket.get_one().await; let config = rocket.config().await; // <-- The code I was afraid would not work, but is actually fine.

Since get_one() returns owned data, there is no conflict between calling get_one and other methods.

However, I see at least one place we would "lose" something: on_launch will no longer be able to call get_one. And on_launch shouldn't change to take &mut Rocket, because by that point it is too late to make many of the modifications that &mut Rocket allows.

That's a good point.

I suppose one idea, perhaps a bad one, is to use some form of interior mutability - say a reentrant lock - to make inspect() take an &Rocket instead of an &mut. I believe if we did that, however, we wouldn't need to expose Manifest at all, which is kind of interesting. In some ways, this feels right as .inspect() is logically read-only, and only takes an &mut due to its implementation.

What do you think about this? Are we gaining anything by separating Rocket and Manifest?

say a reentrant lock

I think I can see it in principle, but it has some really awkward consequences:

let conf = rocket.config().await; // so far so good let rocket = rocket.attach(AdHoc::on_attach(|rocket| Ok(rocket::ignite().manage(X)) )); // this attach fairing is "queued" let state = rocket.state::<X>().await; // this must run the attach fairing conf.get_something(); // oops

Effectively, inspect() or functions that call it would now have to panic if outstanding borrows exist - this also includes calls made from on_launch().

SergioBenitez · 2020-06-13T01:09:29Z

contrib/lib/src/databases.rs

 /// # {
-/// let config = database_config("my_db", rocket.config()).unwrap();
+/// let manifest = rocket.inspect().await;


Can all of these use rocket.config().await now?

SergioBenitez · 2020-06-13T01:14:33Z

core/lib/benches/simple-routing.rs

@@ -109,7 +109,7 @@ mod benches {

        // Hold all of the requests we're going to make during the benchmark.
        let mut requests = vec![];
-        for route in client.rocket().routes() {
+        for route in client.manifest().routes() {


I apparently can't comment on lines that haven't been modified, but note that this no longer compiles due to a missing .await on Client::new(). Can we even do async benches?

Ah right, I keep forgetting about the benches (and the test script doesn't include them). In one sense the answer is "yes, of course" - just write block_on somewhere. Doing it in such a way that the benchmarks actually measure what is supposed to be measured is the hard part - as I believe we have discussed, I expect these style of benchmarks will amplify the per-request overhead of async and fail to demonstrate the overall advantage provided by work-stealing.

How do you feel about deferring these fixes until an upcoming PR for a blocking API for Client, so we can compare some approaches?

Sounds good to me!

examples/form_kitchen_sink/src/tests.rs

…abase doc examples.

SergioBenitez requested changes Mar 3, 2020

View reviewed changes

jebrosen mentioned this pull request Mar 4, 2020

Tracking Issue for Async I/O migration #1065

Closed

15 tasks

jebrosen added 3 commits March 5, 2020 08:19

Remove [target.'cfg(debug_assertions)'.dependencies] in contrib Cargo…

c3a7b2f

….toml. This is not supported and is the same as putting the contents in [dependencies] anyway. It became a warning in rust-lang/cargo#7660.

Rename RocketInner to Manifest, remove Inspector in favor of &Manifest.

b1d85c9

jebrosen force-pushed the async-on-attach branch from bbc3ed4 to 6c165fa Compare March 6, 2020 19:57

jebrosen commented Mar 6, 2020

View reviewed changes

core/lib/src/rocket.rs Outdated Show resolved Hide resolved

core/lib/src/local/client.rs Outdated Show resolved Hide resolved

jebrosen added 16 commits March 7, 2020 13:34

Rename Rocket to Manifest in Client.

4fa7a2f

Explain why finish() does not use self.pending.pop().

2d9c7a7

Remove _manifest_mut, was only used once where _take_manifest() is ca…

215a12a

…lled afterwards anyway.

Replace _take_manifest with finish_and_take_manifest.

1e08936

Call inspect early when calling Template::show.

d26341b

style: add some spacing lines

5ed63e5

Make Fairing::on_attach async.

7236001

This transitively requires that `Rocket::inspect`, `Client::new`, and `Client::untracked` also become async.

Change on_attach() to Box the future, lifting that responsibility fro…

020a518

…m the caller.

Explain why finish is not an async fn.

56b9d31

Add a test verifying that attempting to manage the same state type tw…

186c3e1

…ice panics.

Supply the error message when manifest.take() fails.

a1c7549

Add #[rocket::main] attribute.

abb4a5f

`#[rocket::main]` works like `#[rocket::async_test]`, but it uses tokio's multithreaded scheduler.

Rename entry_point to async_entry.

142ad23

Isolate differences between async_test and main attribute implementat…

46ece3b

…ions to a Kind type.

Make launch() an async fn, replacing serve().

fe65528

Re-enable 'cargo update' in test script.

70b9c38

jebrosen force-pushed the async-on-attach branch from 6c165fa to 70b9c38 Compare March 10, 2020 15:03

SergioBenitez requested changes Jun 4, 2020

View reviewed changes

jebrosen added 2 commits June 5, 2020 11:29

Apply minor changes.

ed14a44

Add a better explanation for finish().

b0dc955

Remove extraneous dependency on futures-util.

3a8c339

jebrosen commented Jun 5, 2020

View reviewed changes

jebrosen added 3 commits June 5, 2020 13:42

Bump parking_lot dependency to a version that compiles on the latest …

fda2aef

…nightly. This partially (re-)applies f4bb8bb

Add 'config()' and 'state()' functions directly to 'Rocket' for conve…

c94e53d

…nience.

Update UI tests for latest nightly.

f881973

SergioBenitez requested changes Jun 6, 2020

View reviewed changes

jebrosen added 2 commits June 6, 2020 14:14

Fix Windows CI by forcing 64-bit rustup installation.

a74798b

Rename 'finish' to 'actualize_manifest'.

94ab440

jebrosen mentioned this pull request Jun 7, 2020

Fully update documentation for 0.5 #1329

Closed

7 tasks

Remove unnecessary 'move' in several 'async move'.

f64d356

SergioBenitez approved these changes Jun 13, 2020

View reviewed changes

jebrosen added 3 commits June 13, 2020 08:36

Use rocket.config() instead of rocket.inspect().await.config() in dat…

f535665

…abase doc examples.

Fix copy/paste error: config -> state.

34bbb22

Convert assertions in form_kitchen_sink tests to use macros.

b8eb258

jebrosen closed this Jun 14, 2020

jebrosen mentioned this pull request Jun 14, 2020

Async: on_attach fairings #1342

Merged

jebrosen deleted the async-on-attach branch June 15, 2020 01:02

SergioBenitez mentioned this pull request Jun 16, 2020

Add '#[rocket::launch]' attribute. #1347

Closed

SergioBenitez added the pr: closed This pull request was not merged label Jun 22, 2020

Async: on_attach fairings #1242

Async: on_attach fairings #1242

Conversation

jebrosen commented Feb 26, 2020

SergioBenitez commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen commented Mar 6, 2020

jebrosen commented Mar 10, 2020

SergioBenitez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez commented Jun 6, 2020 • edited Loading

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen commented Jun 7, 2020

jebrosen commented Jun 7, 2020

SergioBenitez commented Jun 13, 2020 • edited Loading

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez left a comment •

edited

Loading

SergioBenitez commented Jun 6, 2020 •

edited

Loading

SergioBenitez commented Jun 13, 2020 •

edited

Loading