feat: make `TerminatableTask` terminate itself when dropped #1151

YuanYuYuan · 2024-06-17T08:55:03Z

This PR introduces an auto termination when dropped and resolves the bug in #1054.

YuanYuYuan · 2024-06-17T11:03:26Z

@wyfo could you please review this? Thanks.

wyfo

I see why you changed the receiver type, but I think you should not try to reuse terminate in drop.
Instead, you can use some intermediate function:

async fn terminate_inner(handle: JoinHandle<()>, timeout: Duration) -> bool {
    if tokio::time::timeout(timeout, handle).await.is_err() {
        tracing::error!("Failed to terminate the task");
        return false;
    }
    true
}

And use it like

fn drop(&mut self) {
    if let Some(handle) = self.handle.take() {
        ResolveFuture::new(terminate_inner(handle, std::time::Duration::from_secs(10))).res_sync();
    }
}

fn terminate(mut self, timeout: Duration) -> bool {
    ResolveFuture::new(terminate_inner(self.handle.take().unwrap(), timeout)).res_sync();
}

fn terminate_async(mut self, timeout: Duration) -> bool {
    terminate_inner(self.handle.take().unwrap(), timeout).await
}

wyfo · 2024-06-17T11:10:31Z

commons/zenoh-task/src/lib.rs

            token,
        }
    }

    /// Attempts to terminate the task.
    /// Returns true if task completed / aborted within timeout duration, false otherwise.
-    pub fn terminate(self, timeout: Duration) -> bool {
+    pub fn terminate(&mut self, timeout: Duration) -> bool {


I don't think you should change the receiver type of the method

wyfo · 2024-06-17T11:10:41Z

commons/zenoh-task/src/lib.rs

        ResolveFuture::new(async move { self.terminate_async(timeout).await }).res_sync()
    }

    /// Async version of [`TerminatableTask::terminate()`].
-    pub async fn terminate_async(self, timeout: Duration) -> bool {
+    pub async fn terminate_async(&mut self, timeout: Duration) -> bool {


Idem about the receiver type

YuanYuYuan · 2024-06-17T14:40:33Z

If I'm not mistaken, to take out the value inside an Option requires &mut self. So we need to modify terminate and terminate_async in any way. Furthermore, we discussed with @Mallets today that moving out the ownership is not necessary as the borrow checker only lives in Rust, and mixing deconstruction and undeclaration could cause some conflicts. (You can check Undeckarable for more details.) We could follow the common pattern like https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Receiver.html#method.close. Closing a socket or receiver simply takes &mut self and hence Drop can coexist. I will create another draft PR on this topic further.

In conclusion, I suggest splitting the termination and the deconstruction of the task. @wyfo what do you think?

wyfo · 2024-06-17T16:05:20Z

If I'm not mistaken, to take out the value inside an Option requires &mut self. So we need to modify terminate and terminate_async in any way.

Just use mut self instead of self, and you can still use value semantic.

We could follow the common pattern like https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.Receiver.html#method.close.

This is not the same thing here, as terminate returns a boolean. Do you want to store this boolean to return it on following calls?

Closing a socket or receiver simply takes &mut self and hence Drop can coexist.

There is no issue to coexist with drop and having terminate with self receiver.

YuanYuYuan · 2024-06-18T13:45:57Z

Hi @wyfo , thanks for you feedback.

Yes. That's what I mean we need to make some change in any way. Since I saw you wrote this in your first version, I thought you want to avoid any modification on self.

fn terminate(self, timeout: Duration) -> bool {
    ResolveFuture::new(terminate_inner(self.handle.take().unwrap(), timeout)).res_sync();
}

And I'm fine with the change of fn terminate(mut self).

The remaining issue is do we want to enforce a descrtuction here? This is related to the potential conflict with Drop. Let me explain it.

Originally, we define our TerminatableTask without Option

pub struct TerminatableTask {
    handle: JoinHandle<()>,
    token: CancellationToken,
}

This forbids us from implementing Drop for it. The conflict comes from

async fn terminate_inner(handle: JoinHandle<()>, timeout: Duration) -> bool {
    if tokio::time::timeout(timeout, handle).await.is_err() {
        tracing::error!("Failed to terminate the task");
        return false;
    }
    true
}
impl TerminatableTask {
    pub async fn terminate_async(self, timeout: Duration) -> bool {
        terminate_inner(self.handle, timeout).await  // ERROR: cannot move out of type `TerminatableTask`, which implements the `Drop` trait
    }
}

Passing self into the function potentially trigger a deconstruction or move out itself. There are same issues in the Undeclarable trait.
For instance, we implement Undeclarable trait for the Subscriber, then we can't implement Drop for it anymore.

impl<R> Drop for Subscriber<'_, R> {
    fn drop(&mut self) {
        todo!()
    }
}

(You can check this does cause lots of "cannot move out ..." errors)

So that's why I need to wrap the JoinHandle with a Option to not take out the handle but the inner value.

Back to this PR, either using &mut self or mut self can compile. But we still need to think twice: do we need to embed a deconstruction in terminate? As the example I pasted, closing a socket multiple times is acceptable in real use cases. In this PR, a 2nd termination on a closed task is simply doing nothing. This is exactly the same with the previous behavior in zenoh library (before the tokio porting).

Moreover, it can avoid any conflict with Drop if we implment/modify the TerminatableTask as below one day (which unfortunately has already existed in the case of Undeclarable).

impl TerminatableTask {
    pub async fn terminate_async(mut self, timeout: Duration) -> bool {
        let Self { token, mut handle } = self;  // ERROR: cannot move out of type `TerminatableTask`, which implements the `Drop` trait
        token.cancel();
        if let Some(handle) = handle.take() {
            if tokio::time::timeout(timeout, handle).await.is_err() {
                tracing::error!("Failed to terminate the task");
                return false;
            };
        }
        true
    }
}

BTW, I'm little bit confused with your point "should not try to reuse terminate in drop". TBH, I don't care about the return boolean especially it would report an error once it failed. And it seems that your example don't process the boolean result in Drop as well. Am I missing something?

wyfo · 2024-06-20T10:09:10Z

I give way to your arguments, it's good to me.

* Add NOTE for LowLatency transport. (#1088) Signed-off-by: ChenYing Kuo <[email protected]> * fix: Improve debug messages in `zenoh-transport` (#1090) * fix: Improve debug messages for failing RX/TX tasks * fix: Improve debug message for `accept_link` timeout * chore: Fix `clippy::redundant_pattern_matching` error * Improve pipeline backoff (#1097) * Yield task for backoff * Improve comments and error handling in backoff * Simplify pipeline pull * Consider backoff configuration * Add typos check to CI (#1065) * Fix typos * Add typos check to CI * Start link tx_task before notifying router (#1098) * Fix typos (#1110) * bump quinn & rustls (#1086) * bump quinn & rustls * fix ci windows check * add comments * Fix interface name scanning when listening on IP unspecified for TCP/TLS/QUIC/WS (#1123) Co-authored-by: Julien Enoch <[email protected]> * Enable releasing from any branch (#1136) * Fix cargo clippy (#1145) * Release tables locks before propagating subscribers and queryables declarations to void dead locks (#1150) * Send simple sub and qabl declarations using a given function * Send simple sub and qabl declarations after releasing tables lock * Send simple sub and qabl declarations after releasing tables lock (missing places) * feat: make `TerminatableTask` terminate itself when dropped (#1151) * Fix bug in keyexpr::includes leading to call get_unchecked on empty array UB (#1208) * REST plugin uses unbounded flume channels for queries (#1213) * fix: typo in selector.rs (#1228) * fix: zenohd --cfg (#1263) * fix: zenohd --cfg * ci: trigger * Update zenohd/src/main.rs --------- Co-authored-by: Luca Cominardi <[email protected]> * Fix failover brokering bug reacting to linkstate changes (#1272) * Change missleading log * Fix failover brokering bug reacting to linkstate changes * Retrigger CI --------- Co-authored-by: Luca Cominardi <[email protected]> * Code format * Fix clippy warnings * Code format * Fix Clippy errors from Rust 1.80 (#1273) * Allow unexpected `doc_auto_cfg` flag * Keep never-constructed logger interceptor * Ignore interior mutability of `Resource` * Fix typo * Resolve `clippy::doc-lazy-continuation` errors * Upgrade `[email protected]` to `[email protected]` See time-rs/time#693 * Update Cargo.toml (#1277) Updated description to be aligned with what we use everywhere else * Merge ci.yaml --------- Signed-off-by: ChenYing Kuo <[email protected]> Co-authored-by: ChenYing Kuo (CY) <[email protected]> Co-authored-by: Mahmoud Mazouz <[email protected]> Co-authored-by: Tavo Annus <[email protected]> Co-authored-by: JLer <[email protected]> Co-authored-by: Julien Enoch <[email protected]> Co-authored-by: OlivierHecart <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> Co-authored-by: Diogo Matsubara <[email protected]> Co-authored-by: OlivierHecart <[email protected]> Co-authored-by: kydos <[email protected]>

* Add NOTE for LowLatency transport. (#1088) Signed-off-by: ChenYing Kuo <[email protected]> * fix: Improve debug messages in `zenoh-transport` (#1090) * fix: Improve debug messages for failing RX/TX tasks * fix: Improve debug message for `accept_link` timeout * chore: Fix `clippy::redundant_pattern_matching` error * Improve pipeline backoff (#1097) * Yield task for backoff * Improve comments and error handling in backoff * Simplify pipeline pull * Consider backoff configuration * Add typos check to CI (#1065) * Fix typos * Add typos check to CI * Start link tx_task before notifying router (#1098) * Fix typos (#1110) * bump quinn & rustls (#1086) * bump quinn & rustls * fix ci windows check * add comments * Fix interface name scanning when listening on IP unspecified for TCP/TLS/QUIC/WS (#1123) Co-authored-by: Julien Enoch <[email protected]> * Enable releasing from any branch (#1136) * Fix cargo clippy (#1145) * Release tables locks before propagating subscribers and queryables declarations to void dead locks (#1150) * Send simple sub and qabl declarations using a given function * Send simple sub and qabl declarations after releasing tables lock * Send simple sub and qabl declarations after releasing tables lock (missing places) * feat: make `TerminatableTask` terminate itself when dropped (#1151) * Fix bug in keyexpr::includes leading to call get_unchecked on empty array UB (#1208) * REST plugin uses unbounded flume channels for queries (#1213) * fix: typo in selector.rs (#1228) * fix: zenohd --cfg (#1263) * fix: zenohd --cfg * ci: trigger * Update zenohd/src/main.rs --------- Co-authored-by: Luca Cominardi <[email protected]> * Fix failover brokering bug reacting to linkstate changes (#1272) * Change missleading log * Fix failover brokering bug reacting to linkstate changes * Retrigger CI --------- Co-authored-by: Luca Cominardi <[email protected]> * Code format * Fix clippy warnings * Code format * Fix Clippy errors from Rust 1.80 (#1273) * Allow unexpected `doc_auto_cfg` flag * Keep never-constructed logger interceptor * Ignore interior mutability of `Resource` * Fix typo * Resolve `clippy::doc-lazy-continuation` errors * Upgrade `[email protected]` to `[email protected]` See time-rs/time#693 * Update Cargo.toml (#1277) Updated description to be aligned with what we use everywhere else * fix: typos (#1297) * Replace trees computation tasks with a worker (#1303) * Replace trees computation tasks with a worker * Address review comments * Remove review comments * zenohd-default config error #1292 (#1298) * Zenohd panic when tring load file When zenohd trying load file, if it have a problem it crash cause another treat was "unwrap", and it return to a type config. So, it crash and cause painic. * zenohd default config error #1292 When tring load config file defined by -c option. With haver any problema "unwrap" has been to Config type. I treat it return a Default Config whe it happen * If file fail when try load configs If file fail when try load configs * Update main.rs * Resolve typos at comment Resolve typos at comment * fix: typos (#1297) * zenohd-default config error #1292 (#1298) * Zenohd panic when tring load file When zenohd trying load file, if it have a problem it crash cause another treat was "unwrap", and it return to a type config. So, it crash and cause painic. * zenohd default config error #1292 When tring load config file defined by -c option. With haver any problema "unwrap" has been to Config type. I treat it return a Default Config whe it happen * If file fail when try load configs If file fail when try load configs * Update main.rs * Resolve typos at comment Resolve typos at comment * Replace trees computation tasks with a worker (#1303) * Replace trees computation tasks with a worker * Address review comments * Remove review comments * revering fix #1298 --------- Signed-off-by: ChenYing Kuo <[email protected]> Co-authored-by: ChenYing Kuo (CY) <[email protected]> Co-authored-by: Mahmoud Mazouz <[email protected]> Co-authored-by: Luca Cominardi <[email protected]> Co-authored-by: Tavo Annus <[email protected]> Co-authored-by: JLer <[email protected]> Co-authored-by: Julien Enoch <[email protected]> Co-authored-by: OlivierHecart <[email protected]> Co-authored-by: Yuyuan Yuan <[email protected]> Co-authored-by: Diogo Matsubara <[email protected]> Co-authored-by: OlivierHecart <[email protected]> Co-authored-by: kydos <[email protected]> Co-authored-by: brianPA <[email protected]> Co-authored-by: Tiago Neves <[email protected]>

feat: make TerminatableTask terminate itself when dropped

c26caa4

wyfo reviewed Jun 17, 2024

View reviewed changes

Mallets approved these changes Jun 20, 2024

View reviewed changes

Mallets merged commit 2500e5a into eclipse-zenoh:main Jun 20, 2024
12 checks passed

YuanYuYuan deleted the feat/auto-terminate-task-when-drop branch January 9, 2025 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make `TerminatableTask` terminate itself when dropped #1151

feat: make `TerminatableTask` terminate itself when dropped #1151

YuanYuYuan commented Jun 17, 2024

YuanYuYuan commented Jun 17, 2024

wyfo left a comment •

edited

Loading

wyfo Jun 17, 2024

wyfo Jun 17, 2024

YuanYuYuan commented Jun 17, 2024

wyfo commented Jun 17, 2024

YuanYuYuan commented Jun 18, 2024

wyfo commented Jun 20, 2024

feat: make TerminatableTask terminate itself when dropped #1151

feat: make TerminatableTask terminate itself when dropped #1151

Conversation

YuanYuYuan commented Jun 17, 2024

YuanYuYuan commented Jun 17, 2024

wyfo left a comment • edited Loading

Choose a reason for hiding this comment

wyfo Jun 17, 2024

Choose a reason for hiding this comment

wyfo Jun 17, 2024

Choose a reason for hiding this comment

YuanYuYuan commented Jun 17, 2024

wyfo commented Jun 17, 2024

YuanYuYuan commented Jun 18, 2024

wyfo commented Jun 20, 2024

feat: make `TerminatableTask` terminate itself when dropped #1151

feat: make `TerminatableTask` terminate itself when dropped #1151

wyfo left a comment •

edited

Loading