-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
the10thWiz
wants to merge
6
commits into
rwf2:master
Choose a base branch
from
the10thWiz:drop-in-async-launch
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0268007
Drop Rocket inside the tokio async context when using launch
the10thWiz 24a84d5
Add test to validate new behavior
the10thWiz 8243004
Update tests to prevent collisions
the10thWiz f803288
Update tests to set secret key so they work on release
the10thWiz 8129322
Update tests to use unnumbered ports
the10thWiz bd3715c
cleanup and add new test
SergioBenitez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 byrocket::execute
, and then dropped outside of it (and the async runtime). For 0.6, we should consider changing the interface ofrocket::execute
tofn execute<P>(f: impl Future<Output = Rocket<P>>) -> Result<(), Error>
, and calling launch internally.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 asrocket::execute(build().launch())
right now, but drop the rocket instance before returning, and the return type would beResult<(), Error>
instead ofResult<Rocket<Ignite>, Error>
. This would have the effect of makingexecute
only be useful for launching a Rocket instance.We could (equivalently) change
Rocket::launch()
to evaluate toResult<(), 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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.