Is this safe or idiomatic for cleanup in Drop?

code review
1

TL;DR

I'm seeking feedback on the following design pattern for automatically tearing down preset data during unit testing. I'm not sure whether this approach is considered bad practice in the context of Rust testing.

Details

I've looked into various resources and considered the trade-offs. My main motivation is to make the test variable self-responsible for its own cleanup, rather than relying on the user to remember to explicitly call a cleanup method.

I came across this closure-style solution, which does achieve the goal, but I find it a bit verbose in practice.

Therefore, I'm hoping to gather insights or suggestions from more experienced Rust developers. Is there a cleaner or more idiomatic way to approach this? Or is the strategy of embedding async teardown logic in Drop inherently flawed?

Thanks in advance!

use std::{
    sync::{Arc, atomic::{AtomicBool, Ordering}},
    thread::sleep,
    time::Duration,
};

impl Drop for SeedingStruct {
    fn drop(&mut self) {
        let is_done = Arc::new(AtomicBool::new(false));
        let is_done_clone = Arc::clone(&is_done);
        let cloned_self = self.clone();

        let task = async move {
            Resource::delete_by_id(None, cloned_self.resource_id)
                .await
                .expect("Failed to delete resource by its ID");
            is_done_clone.store(true, Ordering::SeqCst);
        };

        // Spawn async task
        let _ = tokio::spawn(task);

        // Busy-wait until the task completes
        while !is_done.load(Ordering::SeqCst) {
            sleep(Duration::from_millis(1));
        }
    }
}
2

this is often referred to as callback API, CPS style API, or scoped API.

if your code relies on the cleanup code to be run, a scoped API is the only reliable way to guarantee that, because the destructor of a guard object may not be run at all (in rust, you can leak the guard, intentionally or accidentally, without unsafe)

I wouldn't say it is "inherently" flawed, it's more of an API design decision I think. ideally, the business logic code should be async-agnostic, but practically, it's not always the best to "abstract" away the async/sync difference, and due to the function coloring problem, sometimes it's inevitable to mix async and non-async code together.

you don't need to use busy loop for this, async runtimes should have some kind of "blocking" API, for tokio, it's Runtime::block_on(), or Handle::block_on(), for smol, it's smol::block_on or async_io::block_on(). so, you can simply do:

// if you are inside a tokio context
Handle::current().block_on(
    Resource::delete_by_id(None, cloned_self.resource_id)
).expect("Failed to delete resource by its ID");
// if not within a runtime context, Handle::current() will panic
// need to create a new runtime
Runtime::new().unwrap().block_on(
    Resource::delete_by_id(None, cloned_self.resource_id)
).expect("");
2 Likes
3

Dear @nerditation

It's always nice to have your feedback.
Thank you. I feel like I made a little progress today :folded_hands: