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

Drop Rocket inside the tokio async context when using launch #2822

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/codegen/src/attribute/entry/launch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ impl EntryAttr for Launch {
#[allow(dead_code)] #f

#vis #sig {
#_error::Error::report(::rocket::async_main(#launch))
::rocket::async_main(async move {
#_error::Error::report(#launch.await)
})
}
))
}
Expand Down
67 changes: 67 additions & 0 deletions core/lib/tests/drop-in-async-context.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#[macro_use]
extern crate rocket;

use rocket::{Build, Config, Rocket};
use rocket::fairing::AdHoc;
use rocket::figment::Figment;

struct AsyncDropInAsync;

impl Drop for AsyncDropInAsync {
fn drop(&mut self) {
// Ensure that managed state is dropped inside of an async context by
// ensuring that we do not panic when fetching the current runtime.
//
// Crates like rocket_sync_db_pools spawn tasks to asynchronously
// complete pool shutdown which must be done in an async context or else
// the spawn will panic. We want to ensure that does not happen.
let _ = rocket::tokio::runtime::Handle::current();
}
}

fn rocket() -> Rocket<Build> {
let figment = Figment::from(Config::debug_default())
.merge(("address", "tcp:127.0.0.1:0"));

rocket::custom(figment)
.manage(AsyncDropInAsync)
.attach(AdHoc::on_liftoff("Shutdown", |rocket| Box::pin(async {
rocket.shutdown().notify();
})))
}

mod launch {
#[launch]
fn launch() -> _ {
super::rocket()
}

#[test]
fn test_launch() {
main();
}
}

mod main {
#[rocket::main]
async fn main() {
super::rocket().launch().await.unwrap();
}

#[test]
fn test_main() {
main();
}

#[test]
fn test_execute() {
rocket::execute(async {
super::rocket().launch().await.unwrap();
});
}

#[test]
fn test_execute_directly() {
rocket::execute(super::rocket().launch()).unwrap();
}
Comment on lines +64 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test, which I think should pass for the same reasons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's happening here, is Rocket is created and launched, on the async runtime. However, launch returns Result<Rocket<Ignite>, Error>. In the success case, Rocket is then returned by rocket::execute, and then dropped outside of it (and the async runtime). For 0.6, we should consider changing the interface of rocket::execute to fn execute<P>(f: impl Future<Output = Rocket<P>>) -> Result<(), Error>, and calling launch internally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why the one I wrote explicitly calls unwrap inside an async block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main use-case is exactly calling it with rocket.launch() directly, so it should be able to do that without any footguns or extra ceremony.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue with execute right now is that it returns the Rocket instance. The runtime only exists inside the execute function, so it has to be dropped before returning.

For my proposal, rocket::execute(build()) would be roughly the same as rocket::execute(build().launch()) right now, but drop the rocket instance before returning, and the return type would be Result<(), Error> instead of Result<Rocket<Ignite>, Error>. This would have the effect of making execute only be useful for launching a Rocket instance.

We could (equivalently) change Rocket::launch() to evaluate to Result<(), Error>. This would also allow reverting the changes in the launch macro, since the launch method would take of dropping the Rocket instance in the async context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's a better approach altogether. In addition to trying to drop Rocket in an async context, why not also specifically drop managed state on shutdown? That is, as part of the shutdown procedure, which is async, drop managed state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that might be a valid approach, but the tradeoff would be that we can't support relaunching a Rocket application after it's been shutdown.

Given all this, what is the value to Rocket returning the instance after shutdown? The two values I can see is a) relaunching Rocket after shutdown, and b) inspecting managed state after shutdown. The fact that Rocket is dropped in the error case mean it's likely not super useful to inspect managed state, since that's when you would actually want to inspect it. I'm not sure what the exact use case of relaunching Rocket is, and I'm not convinced it wouldn't be better to recommend simply building a new instance of Rocket.

I think the best compromise would be changing launch to not return the Rocket instance, ever. This ensures Rocket is dropped inside an async context (since it's dropped before the launch future completes). Alternatively, we could create a Shutdown phase to return, which would have already dropped managed state, and be safe drop outside an async context.

}
Loading