Change: remove AsyncRuntime::abort()

[drmingdrmer]

Changelog

Change: remove AsyncRuntime::abort()

Not all of the async-runtime provide an abort() primitive, such as
monoio.
In this commit abort() is removed in order to allow Openraft to be
compatible with monoio.

Tick is the only mod that relies on abort() for shutdown.
In this commit shutting down is replaced with using tokio::sync::oneshot::channel.

Refer to:

• Add a Monoio async runtime #1010 (review)

---

This change is 

[drmingdrmer]

@Miaxos
Hope this would address the issue with your PR:)

[schreter]

Yes, using a channel instead of abort() is much cleaner. I just added some minor comments.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ariesdevil, @drmingdrmer, and @lichuang)

---

openraft/src/core/tick.rs line 41 at r1 (raw file):

    cancel: Mutex<Option<oneshot::Sender<()>>>,
    #[allow(dead_code)]
    join_handle: JoinHandleOf<C, ()>,

Do you need the join handle at all?

---

openraft/src/core/tick.rs line 64 at r1 (raw file):

        TickHandle {
            enabled,
            cancel: Mutex::new(Some(cancel)),

I'd move mutex creation before spawn(). If the mutex creation fails here (e.g., due to OOM), then the tick task might continue to run forever.

On a similar note, a Drop for TickHandle might make sense as well to send the stop signal if the tick task was not stopped yet. OTOH, it should fail sending the tick to main loop, since likely the receiver of the tick will be destroyed by then.

Code quote:

Mutex::new(Some(cancel))

---

openraft/src/core/tick.rs line 78 at r1 (raw file):

            let at = <C::AsyncRuntime as AsyncRuntime>::Instant::now() + self.interval;
            let mut sleep_fut = AsyncRuntimeOf::<C>::sleep_until(at);

BTW, Interval might be a better abstraction here (but we need to add it to the runtime; tokio::time::Interval only works with Tokio runtime).

---

openraft/src/core/tick.rs line 93 at r1 (raw file):

            if !self.enabled.load(Ordering::Relaxed) {
                i -= 1;

Unrelated: wouldn't it make sense to move i += 1 from the top of the loop below this if instead of decrementing again here?

---

openraft/src/core/tick.rs line 126 at r1 (raw file):

        } else {
            tracing::info!("Timer cancel signal is already sent");
        }

Maybe adjust trace messages a bit?

Suggestion:

        if let Some(cancel) = got {
            let send_res = cancel.send(());
            tracing::debug!("Timer cancel signal sent: {send_res:?}");
        } else {
            tracing::warn!("Double call to Raft::shutdown()");
        }

[drmingdrmer]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ariesdevil, @lichuang, and @schreter)

---

openraft/src/core/tick.rs line 41 at r1 (raw file):

Previously, schreter wrote…

Do you need the join handle at all?

Hmm. It should be returned from shutdown() and let the caller to block waiting for the timer until completely shutdown.

---

openraft/src/core/tick.rs line 64 at r1 (raw file):

Previously, schreter wrote…

I'd move mutex creation before spawn(). If the mutex creation fails here (e.g., due to OOM), then the tick task might continue to run forever.

On a similar note, a Drop for TickHandle might make sense as well to send the stop signal if the tick task was not stopped yet. OTOH, it should fail sending the tick to main loop, since likely the receiver of the tick will be destroyed by then.

Good point!

---

openraft/src/core/tick.rs line 78 at r1 (raw file):

Previously, schreter wrote…

BTW, Interval might be a better abstraction here (but we need to add it to the runtime; tokio::time::Interval only works with Tokio runtime).

🤔

---

openraft/src/core/tick.rs line 93 at r1 (raw file):

Previously, schreter wrote…

Unrelated: wouldn't it make sense to move i += 1 from the top of the loop below this if instead of decrementing again here?

Yes. I can't recall why it was like that. :(

---

openraft/src/core/tick.rs line 126 at r1 (raw file):

Previously, schreter wrote…

Maybe adjust trace messages a bit?

Usually the timer cancel event will only happen once for each process, and I'd like to keep the key component start/end event at info level.

The warning level makes sense. :)

[schreter]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

---

openraft/src/core/tick.rs line 41 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Hmm. It should be returned from shutdown() and let the caller to block waiting for the timer until completely shutdown.

Makes sense.

---

openraft/src/core/tick.rs line 49 at r2 (raw file):

    /// Signal the tick loop to stop, without waiting for it to stop.
    fn drop(&mut self) {
        let _ = self.shutdown();

Well, this will cause the warning message to be written after a correct shutdown. You should only call shutdown() if it wasn't done so by a proper call to shutdown() by the user.

[drmingdrmer]

Reviewable status: all files reviewed, 2 unresolved discussions

---

openraft/src/core/tick.rs line 49 at r2 (raw file):

Previously, schreter wrote…

Well, this will cause the warning message to be written after a correct shutdown. You should only call shutdown() if it wasn't done so by a proper call to shutdown() by the user.

You are right. Let me fix it.