Drop Rocket inside the tokio async context when using launch #2822
Conversation
the10thWiz
force-pushed
the
drop-in-async-launch
branch
2 times, most recently
from
c905bcb
to
b47211c
Compare
| mod main { | ||
| use rocket::tokio::net::TcpListener; | ||
|
|
||
| #[rocket::main] | ||
| async fn main() { | ||
| super::rocket() | ||
| .try_launch_on(TcpListener::bind("localhost:8001")) | ||
| .await | ||
| .unwrap(); | ||
| } | ||
| #[test] | ||
| fn test_main() { | ||
| main(); | ||
| } | ||
| #[test] | ||
| fn test_execute() { | ||
| rocket::execute(async { | ||
| super::rocket() | ||
| .try_launch_on(TcpListener::bind("localhost:8002")) | ||
| .await | ||
| .unwrap(); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is extremely fragile. The only correct way to start a TCP server in tests is to do so with an unnumbered port.
Given what this code is doing, I really believe this needs to be a test in testbench. The tests are easier to write this way, too. All you need to do is introduce a new module in testbench/src/servers. The returned handle allows you to shutdown the server as well.
There was a problem hiding this comment.
Yeah, I should switch to using an unnumbered port.
That being said, I'm not confident I can use testbench for this specific case - at least not without some issues. The challenge is that I'm trying to test the #[launch] macro itself - it looks like most cases pass a rocket instance to the spawn macro, which then attaches a fairing. I can't do that - since I'm directly executing the generated (sync) main function.;
There was a problem hiding this comment.
I've taken the time to make an attempt (it required some modifications to testbench, and still doesn't work). You can check it out from the 'drop-in-async-launch-testbench' branch of my fork.
the10thWiz
force-pushed
the
drop-in-async-launch
branch
from
1d46855
to
8742c65
Compare
SergioBenitez
force-pushed
the
drop-in-async-launch
branch
2 times, most recently
from
47df51d
to
4b2142a
Compare
| fn test_execute_directly() { | ||
| rocket::execute(super::rocket().launch()).unwrap(); | ||
| } |
There was a problem hiding this comment.
I added this test, which I think should pass for the same reasons?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is why the one I wrote explicitly calls unwrap inside an async block.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
Moves error printing and the implicit drop inside the tokio runtime when using the `#[launch]` attribute.
SergioBenitez
force-pushed
the
drop-in-async-launch
branch
from
4b2142a
to
bd3715c
Compare
Moves error printing and the implicit drop inside the tokio runtime when using the
#[launch]attribute.Based on an issue reported on Element. See https://matrix.to/#/!kDIcCXWSVfdahNCJWq:mozilla.org/$2CSzlNC4rQw7Wd7Y7L2fV-2_fTxsvq93elL1foVfQ1s?via=mozilla.org&via=matrix.org&via=catgirl.cloud.
Basically, the Rocket instance gets dropped (after a successful shutdown or a
Shutdownerror) outside the tokio runtime. Some minor restructuring of the generated code ensures the tokio context outlives Rocket, if only slightly.