feat(jstzd): use AsyncDropper in JstzdServer

[huancheng-trili]

Context

Closes JSTZ-121.

JSTZ-121

Description

Wrap the test in a future so that we can still call stop at the end of the test to clean up everything before the program exits should the test fail and panic. Otherwise, the test program does not actually close after the panic and the baker and octez-node processes remain running even after the test program terminates.

In a tokio task, panic is captured and returned as Err, so handling failed assertions like this is much simpler than trying to do something like catch_unwind.

Manually testing the PR

The test should still work as is.

[codecov]

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.

Project coverage is 45.85%. Comparing base (dc006a6) to head (416eae3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/jstzd/src/task/jstzd.rs	76.47%	2 Missing and 6 partials ⚠️

Files with missing lines	Coverage Δ
crates/jstzd/src/task/jstzd.rs	77.29% <76.47%> (+3.58%)	⬆️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc006a6...416eae3. Read the comment docs.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[zcabter]

Isn't the problem because JstzServer, although it implements AsyncDrop, isn't instantiated with AsyncDropper<JstzServer>?
Since AsyncDropper requires impl Default, ServerState.jstzd_config field might have to wrapped in Option<> which is rather inconvenient :(

If there is a jstzd panics for some reason, we definitely want the octez resources to be cleaned up. Is there a better way to to hook self.stop()when the server starts up?

[huancheng-trili]

Isn't the problem because JstzServer, although it implements AsyncDrop, isn't instantiated with AsyncDropper?

Yes. I thought implementing AsyncDrop would be sufficient and forgot about AsyncDropper. I've changed this PR and added AsyncDropper to JstzServer. The implementation might look a bit awkward because of the locks. Ideally AsyncDrop should be implemented for ServerState, but there will be some borrow_mut issues without having the lock. The only way to get the lock involved is to wrap everything with a struct and implement AsyncDrop for that struct instead. Now AsyncDropper is embedded in JstzServer. At least with this we don't need to write AsyncDropper<JstzServer> everywhere.

Since AsyncDropper requires impl Default, ServerState.jstzd_config field might have to wrapped in Option<> which is rather inconvenient :(

That's true, but it's not that inconvenient since the server does not really access jstzd_config that much. I think it's okay to just wrap it.

If there is a jstzd panics for some reason, we definitely want the octez resources to be cleaned up. Is there a better way to to hook self.stop()when the server starts up?

AsyncDrop should be able to handle what we can handle for now. Based on my observation, AsyncDrop for JstzdServer does clean up octez resources in the following cases:

• when something in the program outside of JstzdServer panics and causes the program to terminate
• when something inside JstzdServer itself, e.g. some route handler, panics
• when something inside Jstzd itself, e.g. something bad in any of its methods, panics

At this moment, there aren't really that many places in Jstzd where it can panic. It has only a few methods and AsyncDrop for JstzdServer can handle failures at those places. For spawn, after calling spawn, the child processes continue on their owns and Jstzd does not get affected should those processes fail somehow. I think there was some discussion before regarding making jstzd "recover" from subprocess failures and we can look into this later on, but for now Jstzd won't be aware of those events.

[zcabter]

LGTM thanks