Feature: Add Raft::external_state_machine_request() to run a function inside state machine

[drmingdrmer]

Changelog

Feature: Add Raft::external_state_machine_request() to run a function inside state machine

Add Raft::external_sm_request() in a fire-and-forget manner,
and Raft::with_state_machine() blocks waiting on the response.

Other changes: move OptionalSerde, OptionalSync, OptionalSync from
openraft:: to openraft::base::

---

This change is 

[schreter]

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)

---

openraft/src/core/raft_core.rs line 1220 at r1 (raw file):

                        let res = self.sm_handle.send(sm_cmd);
                        if let Err(e) = res {
                            tracing::error!(error = display(e), "error sending sm::Command to sm::Worker");

BTW, I noticed you use display() at many places. You know there are also shortcuts in tracing, like %x for display()?

OTOH, display() is more speaking than some random % character...

Suggestion:

tracing::error!(error = %e, "error sending sm::Command to sm::Worker");

---

openraft/src/raft/mod.rs line 831 at r1 (raw file):

    ///
    /// The request functor will be called with a mutable reference to the state machine
    /// and.

and what? (docs)

Code quote:

    /// The request functor will be called with a mutable reference to the state machine
    /// and.

[drmingdrmer]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter)

---

openraft/src/core/raft_core.rs line 1220 at r1 (raw file):

Previously, schreter wrote…

BTW, I noticed you use display() at many places. You know there are also shortcuts in tracing, like %x for display()?

OTOH, display() is more speaking than some random % character...

I know that but some IDE fails to parse %e/?e and emit an error.
I prefer to use function call like syntax if possible. 🤔

---

openraft/src/raft/mod.rs line 831 at r1 (raw file):

Previously, schreter wrote…

and what? (docs)

😟

[schreter]

Reviewed 8 of 9 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/base/mod.rs line 26 at r3 (raw file):

    pub type BoxAsyncOnceMut<'a, A, T = ()> = Box<dyn FnOnce(&mut A) -> BoxFuture<T> + Send + 'a>;
    pub type BoxOnce<'a, A, T = ()> = Box<dyn FnOnce(&A) -> T + Send + 'a>;
    pub type BoxAny = Box<dyn Any + Send>;

Another option would be to create a helper trait Any + OptionalSend in common code and implement it for all T, but this is also fine. However, see below.

---

openraft/src/core/raft_core.rs line 1220 at r1 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I know that but some IDE fails to parse %e/?e and emit an error.
I prefer to use function call like syntax if possible. 🤔

No problem, I just wanted to point it out. And yes, display() is more speaking.

BTW, IDEs my colleagues are using (VS Code, VIM and Emacs) all render it correctly.

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

                    let _ = self.resp_tx.send(Notification::sm(res));
                }
                Command::Func { func, input_sm_type } => {

Regarding the trait above, the helper trait for func would of course also allow you to define and auto-implement methods, like type_name(), so you don't need to carry type name with you.

---

openraft/src/core/sm/worker.rs line 154 at r3 (raw file):

                    } else {
                        tracing::warn!(
                            "User defined SM function uses incorrect state machine type, expect: {}, got: {}",

?

Suggestion:

"User-defined SM function uses incorrect state machine type, expected: {}, got: {}"

---

openraft/src/raft/mod.rs line 800 at r3 (raw file):

    ///
    /// If the user function fail to run, e.g., the input `SM` is different one from the one in
    /// `RaftCore`, it returns a `&'static str` error.

And what is in the &'static str? I'd suggest to return a wrapper, such as struct InvalidStateMachineType { pub actual_type: &'static str };.

---

openraft/src/raft/mod.rs line 832 at r3 (raw file):

        let Ok(v) = recv_res else {
            if self.inner.is_core_running().await {
                return Ok(Err("State machine user defined function fail to execute."));

?

Suggestion:

User-defined function on the state machine failed to execute

[drmingdrmer]

Reviewed 9 of 9 files at r2, 1 of 1 files at r3.
Reviewable status: 13 of 16 files reviewed, 2 unresolved discussions (waiting on @schreter)

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, schreter wrote…

Regarding the trait above, the helper trait for func would of course also allow you to define and auto-implement methods, like type_name(), so you don't need to carry type name with you.

It looks like such an approach only outputs "alloc::boxed::Box<dyn core::any::Any>"
Is there anything I missed?

use std::any::Any;

trait Foo: Any {
    fn get_type(&self) -> &'static str;
}

impl<T: Any> Foo for T {
    fn get_type(&self) -> &'static str {
        std::any::type_name::<T>()
    }
}

fn main() {
    let a: Box<dyn Any> = Box::new(5);
    println!("{}", a.get_type());
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7058005d5cead64910f983dc1d8145d2

---

openraft/src/core/sm/worker.rs line 154 at r3 (raw file):

Previously, schreter wrote…

?

Thanks for these minor typo fixes!

[schreter]

Reviewed 3 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

It looks like such an approach only outputs "alloc::boxed::Box<dyn core::any::Any>"
Is there anything I missed?

use std::any::Any;

trait Foo: Any {
    fn get_type(&self) -> &'static str;
}

impl<T: Any> Foo for T {
    fn get_type(&self) -> &'static str {
        std::any::type_name::<T>()
    }
}

fn main() {
    let a: Box<dyn Any> = Box::new(5);
    println!("{}", a.get_type());
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7058005d5cead64910f983dc1d8145d2

Well, of course, since it's implemented for Any, it's also implemented for Box, so it won't auto-deref. I.e., you are printing the type of the Box<dyn Any> (in fact, it should read Box<dyn Foo>), not of the contained T.

If you'd rewrite it with a.as_ref().get_type() or similar (dereferencing the Box), you'd see the correct integer type.

Unfortunately, Rust playground doesn't work at the moment, so I can't update the code there

---

openraft/src/error/invalid_sm.rs line 15 at r5 (raw file):

        )
    }
}

?

Suggestion:

#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
#[error("User-defined function on the state machine failed to run; It may have used a different type \
         of state machine from the one in RaftCore (`{actual_type}`)")]
pub struct InvalidStateMachineType {
    pub actual_type: &'static str,
}

[drmingdrmer]

Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @schreter)

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, schreter wrote…

Well, of course, since it's implemented for Any, it's also implemented for Box, so it won't auto-deref. I.e., you are printing the type of the Box<dyn Any> (in fact, it should read Box<dyn Foo>), not of the contained T.

If you'd rewrite it with a.as_ref().get_type() or similar (dereferencing the Box), you'd see the correct integer type.

Unfortunately, Rust playground doesn't work at the moment, so I can't update the code there

It looks like as_ref() yield type dyn Any but not the type behind Any?

[schreter]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

It looks like as_ref() yield type dyn Any but not the type behind Any?

That's correct. But the implementation of Foo on this type (i32) is that returning "i32". Note that the trait implementation is generated for the concrete type. The call on dyn Trait then dispatches the call to the generated function hidden behind object type dyn Trait. Also, note that the Box is then not on dyn Any but rather on dyn Foo, since that's the type you require. But,

See the fixed example at https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c305a6e746a978c99fc0e845b5d3cca, it will print i32.

[drmingdrmer]

Reviewable status: all files reviewed, 1 unresolved discussion

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, schreter wrote…

That's correct. But the implementation of Foo on this type (i32) is that returning "i32". Note that the trait implementation is generated for the concrete type. The call on dyn Trait then dispatches the call to the generated function hidden behind object type dyn Trait. Also, note that the Box is then not on dyn Any but rather on dyn Foo, since that's the type you require. But,

See the fixed example at https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9c305a6e746a978c99fc0e845b5d3cca, it will print i32.

I see. But the problem here is that I can not use a customized type like using Foo.
The type here is FnOnce(&mut SM), there is a type parameter SM that must be specified.
But the channel for sending this command(enum RaftMsg<C>) does not have this type parameter.

[schreter]

Reviewable status: all files reviewed, 1 unresolved discussion

---

openraft/src/core/sm/worker.rs line 146 at r3 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I see. But the problem here is that I can not use a customized type like using Foo.
The type here is FnOnce(&mut SM), there is a type parameter SM that must be specified.
But the channel for sending this command(enum RaftMsg<C>) does not have this type parameter.

Sure, but I was suggesting to add something like this:

trait CallSM: OptionalSend {
    fn sm_type(&self) -> &'static str;
    fn call(self, sm: &mut dyn Any) -> Box<dyn Future<Output = Result<(), ...>>>;
}

trait InnerFunc<SM>: FnOnce<&mut SM> + OptionalSend {}
impl<SM, F: FnOnce<&mut SM> + OptionalSend> InnerFunc<SM> for F {}

pub struct Call<SM, F: InnerFunc<SM>>(F);

impl<SM, F: InnerFunc<SM>> CallSM for Call<SM, F> {
    fn sm_type(&self) -> &'static str { std::any::type_name::<SM>() }
    fn call(self, sm: &mut dyn Any) -> Result<Box<dyn Future<Output = ()>>, ...> {
        if let Some(sm) = sm.downcast_ref::<SM>() {
            Ok((self.0)(sm))
        } else {
            Err(...)
        }
    }
}

Then, pack the function into Call and use Box<dyn CallSM> to transfer it. At the receiving side, store the type, then call the function and if it fails, use the type in the error message.