AsyncRuntime::oneshot

[Miaxos]

A draft for #1023 there is a clear colouring of the AsyncRuntime when abstracting it this way, WDYT?

Checklist

• Updated guide with pertinent info (may not always apply).
• Squash down commits to one or two logical commits which clearly describe the work you've done.
• Unittest is a friend:)

---

This change is 

[schreter]

In general, looks good from my POV. You just pushed it under a different PR, so it's now a completely new PR, not reopen of the older closed one.

Reviewed 27 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Miaxos)

---

openraft/src/async_runtime.rs line 50 at r1 (raw file):

    type OneshotSender<T: OptionalSend>: AsyncOneshotSendExt<T> + OptionalSend + OptionalSync + Debug + Sized;

    type OneshotReceiverError: std::error::Error + OptionalSend;

Doc?

---

openraft/src/async_runtime.rs line 102 at r1 (raw file):

pub struct TokioRuntime;

pub struct TokioSendWrapper<T: OptionalSend>(pub tokio::sync::oneshot::Sender<T>);

Maybe simply "TokioOneShotSender"?

---

openraft/src/engine/command.rs line 242 at r1 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

If we use the derive(PartialEq) it forces a constraint PartialEq on every generic, that why I expanded the macro. (same for Eq)

Yes, that's a shortcoming of the derive macro - it doesn't derive if one of the generics doesn't support it. Instead, it should look at the actual members whether they support it. But, that's impossible in a proc-macro, so a special impl would be required in the compiler.

But, I'm fine with that, just maybe name them somewhat more descriptively than "f0" (maybe "channel" or "sender")?

[Miaxos]

Yeah I couldn't reopen it, not sure why.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Miaxos)

[Miaxos]

Reviewable status: 25 of 27 files reviewed, 3 unresolved discussions (waiting on @schreter)

---

openraft/src/async_runtime.rs line 50 at r1 (raw file):

Previously, schreter wrote…

Doc?

Updated

---

openraft/src/async_runtime.rs line 102 at r1 (raw file):

Previously, schreter wrote…

Maybe simply "TokioOneShotSender"?

Changed to TokioOneShotSender

---

openraft/src/engine/command.rs line 242 at r1 (raw file):

Previously, schreter wrote…

Yes, that's a shortcoming of the derive macro - it doesn't derive if one of the generics doesn't support it. Instead, it should look at the actual members whether they support it. But, that's impossible in a proc-macro, so a special impl would be required in the compiler.

But, I'm fine with that, just maybe name them somewhat more descriptively than "f0" (maybe "channel" or "sender")?

Changed to first_sender and second_sender

[schreter]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Miaxos)

[drmingdrmer]

BTW there is a conflict with the main :(

Reviewed 7 of 27 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

pub struct TokioRuntime;

pub struct TokioOneShotSender<T: OptionalSend>(pub tokio::sync::oneshot::Sender<T>);

Why does it need a wrapper for the Sender?

---

openraft/src/core/raft_msg/mod.rs line 31 at r2 (raw file):

/// A oneshot TX to send result from `RaftCore` to external caller, e.g. `Raft::append_entries`.
pub(crate) type ResultSender<Runtime, T, E = Infallible> = <Runtime as AsyncRuntime>::OneshotSender<Result<T, E>>;

Using C: RaftTypeConfig as the type parameter could be better?

---

openraft/src/core/raft_msg/mod.rs line 36 at r2 (raw file):

/// TX for Vote Response
pub(crate) type VoteTx<Runtime, NID> = ResultSender<Runtime, VoteResponse<NID>>;

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

---

openraft/src/core/raft_msg/mod.rs line 39 at r2 (raw file):

/// TX for Append Entries Response
pub(crate) type AppendEntriesTx<Runtime, NID> = ResultSender<Runtime, AppendEntriesResponse<NID>>;

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

---

openraft/src/engine/command.rs line 224 at r2 (raw file):

#[derive(Debug)]
#[derive(derive_more::From)]
pub(crate) enum Respond<R, NID, N>

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

---

openraft/src/storage/callback.rs line 16 at r2 (raw file):

/// A oneshot callback for completion of log io operation.
pub struct LogFlushed<Runtime, NID>

If it requires more than one type parameter, use LogFlushed<C: RaftTypeConfig> would be better.

[schreter]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Why does it need a wrapper for the Sender?

I suppose, because of Debug requirement above?

[drmingdrmer]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, schreter wrote…

I suppose, because of Debug requirement above?

But then why does it have to be Debug?

[drmingdrmer]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

But then why does it have to be Debug?

I mean, just manually implement Debug for TokioRuntime would be easier IMHO.

[schreter]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I mean, just manually implement Debug for TokioRuntime would be easier IMHO.

True.

[Miaxos]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, schreter wrote…

True.

OneshotSender<T> needs to be Debug, tokio::sync::oneshot::Sender<T> imp Debug only if T: Debug but it would means we need to add a constraint on T and it would "color" the whole codebase with a Debug constraint on T.

This is not due to TokioRuntime but mainly for every struct with a sender inside which implement Debug (like ValueSender) for instance.

[schreter]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

OneshotSender<T> needs to be Debug, tokio::sync::oneshot::Sender<T> imp Debug only if T: Debug but it would means we need to add a constraint on T and it would "color" the whole codebase with a Debug constraint on T.

This is not due to TokioRuntime but mainly for every struct with a sender inside which implement Debug (like ValueSender) for instance.

Ah, so I was correct it was because of Debug. I didn't think about the other structs - but yes, you are right. That would make the rest quite complex. OTOH, why did it work before with Tokio sender? It's no different from before, where Tokio sender was referenced directly.

Could it be that something else is lacking Debug, like the AsyncRuntime, as @drmingdrmer pointed out? Maybe replacing the type parameter with TypeConfig as indicated elsewhere will resolve that?

[drmingdrmer]

Reviewed 3 of 27 files at r1, 3 of 9 files at r3, 8 of 9 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 25 of 27 files reviewed, 6 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 55 at r5 (raw file):

    let mut eng = eng();

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

Inside Openraft codebase just use AsyncRuntimeOf::<UTConfig> for short.
If needed, you can also create other type alias for the oneshot types, such as OneshotSenderOf<C>.
That will make a little bit easier:)

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 60 at r5 (raw file):

    assert!(resp.is_none());

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

AsyncRuntimeOf::<UTConfig> for short.

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 80 at r5 (raw file):

    let mut eng = eng();

    let (tx, _rx) = <<UTConfig as RaftTypeConfig>::AsyncRuntime as AsyncRuntime>::oneshot();

AsyncRuntimeOf::<UTConfig> for short.

[Miaxos]

Reviewable status: 24 of 28 files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/core/raft_msg/mod.rs line 31 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Using C: RaftTypeConfig as the type parameter could be better?

Done

---

openraft/src/core/raft_msg/mod.rs line 36 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

Done

---

openraft/src/core/raft_msg/mod.rs line 39 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

Done

---

openraft/src/engine/command.rs line 224 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Use C: RaftTypeConfig as the type parameter then it requires only one type parameter.

Done

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 55 at r5 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Inside Openraft codebase just use AsyncRuntimeOf::<UTConfig> for short.
If needed, you can also create other type alias for the oneshot types, such as OneshotSenderOf<C>.
That will make a little bit easier:)

Done

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 60 at r5 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

AsyncRuntimeOf::<UTConfig> for short.

Done

---

openraft/src/engine/handler/vote_handler/accept_vote_test.rs line 80 at r5 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

AsyncRuntimeOf::<UTConfig> for short.

Done

---

openraft/src/storage/callback.rs line 16 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

If it requires more than one type parameter, use LogFlushed<C: RaftTypeConfig> would be better.

Done

[Miaxos]

I needed to fix a commit message, sorry for the rebase

Reviewable status: 21 of 28 files reviewed, 6 unresolved discussions (waiting on @drmingdrmer and @schreter)

[drmingdrmer]

Reviewed 2 of 4 files at r6, 3 of 3 files at r7.
Reviewable status: 26 of 28 files reviewed, 4 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/engine/command.rs line 260 at r7 (raw file):

        }
    }
}

Why does it require a manual implementation of PartialEq?
C::NodeId : PartialEq and C::Node : PartialEq is always satisified:
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L11
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L62

Code quote:

impl<C> PartialEq for Respond<C>
where
    C: RaftTypeConfig,
    C::NodeId: PartialEq,
    C::Node: PartialEq,
{
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Respond::Vote(first_sender), Respond::Vote(second_sender)) => first_sender.eq(second_sender),
            (Respond::AppendEntries(first_sender), Respond::AppendEntries(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::ReceiveSnapshotChunk(first_sender), Respond::ReceiveSnapshotChunk(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::InstallSnapshot(first_sender), Respond::InstallSnapshot(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::InstallFullSnapshot(first_sender), Respond::InstallFullSnapshot(second_sender)) => {
                first_sender.eq(second_sender)
            }
            (Respond::Initialize(first_sender), Respond::Initialize(second_sender)) => first_sender.eq(second_sender),
            _unused => false,
        }
    }
}

---

openraft/src/engine/command.rs line 294 at r7 (raw file):

#[derive(Debug)]
pub(crate) struct ValueSender<R, T>

What about using C: RaftTypeConfig?

It would be easier for developers to use just C instead of using C somewhere and R somewhere else.

[drmingdrmer]

Reviewed 1 of 27 files at r1, 1 of 4 files at r6, 5 of 5 files at r9, all commit messages.
Reviewable status: 27 of 28 files reviewed, 3 unresolved discussions (waiting on @Miaxos and @schreter)

[Miaxos]

Reviewable status: 27 of 28 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/engine/command.rs line 260 at r7 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

Why does it require a manual implementation of PartialEq?
C::NodeId : PartialEq and C::Node : PartialEq is always satisified:
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L11
https://github.com/datafuselabs/openraft/blob/4ad89fefdf6060319d594d4a0fd6f37d6daf43ee/openraft/src/node.rs#L62

The derive macro generated by a derive(PartialEq) imply that C::AsyncRuntime: PartialEq which is not what we want, that is why I switched to the manual impl to have the same behavior as before.

// ==========================================
// Recursive expansion of the PartialEq macro
// ==========================================

impl<C: $crate::cmp::PartialEq> $crate::cmp::PartialEq for Respond<C>
where
    C: RaftTypeConfig,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::Node: $crate::cmp::PartialEq,
{
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Respond::Vote(f0_self), Respond::Vote(f0_other)) => f0_self.eq(f0_other),
            (Respond::AppendEntries(f0_self), Respond::AppendEntries(f0_other)) => f0_self.eq(f0_other),
            (Respond::ReceiveSnapshotChunk(f0_self), Respond::ReceiveSnapshotChunk(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallSnapshot(f0_self), Respond::InstallSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallFullSnapshot(f0_self), Respond::InstallFullSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::Initialize(f0_self), Respond::Initialize(f0_other)) => f0_self.eq(f0_other),
            _unused => false,
        }
    }
}

---

openraft/src/engine/command.rs line 294 at r7 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

What about using C: RaftTypeConfig?

It would be easier for developers to use just C instead of using C somewhere and R somewhere else.

Yep, will do

[Miaxos]

Reviewable status: 23 of 28 files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/engine/command.rs line 294 at r7 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

Yep, will do

Done

[drmingdrmer]

Reviewed 3 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, schreter wrote…

Ah, so I was correct it was because of Debug. I didn't think about the other structs - but yes, you are right. That would make the rest quite complex. OTOH, why did it work before with Tokio sender? It's no different from before, where Tokio sender was referenced directly.

Could it be that something else is lacking Debug, like the AsyncRuntime, as @drmingdrmer pointed out? Maybe replacing the type parameter with TypeConfig as indicated elsewhere will resolve that?

And what about this issue? @Miaxos

---

openraft/src/engine/command.rs line 260 at r7 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

The derive macro generated by a derive(PartialEq) imply that C::AsyncRuntime: PartialEq which is not what we want, that is why I switched to the manual impl to have the same behavior as before.

// ==========================================
// Recursive expansion of the PartialEq macro
// ==========================================

impl<C: $crate::cmp::PartialEq> $crate::cmp::PartialEq for Respond<C>
where
    C: RaftTypeConfig,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::AsyncRuntime: $crate::cmp::PartialEq,
    C::NodeId: $crate::cmp::PartialEq,
    C::Node: $crate::cmp::PartialEq,
{
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Respond::Vote(f0_self), Respond::Vote(f0_other)) => f0_self.eq(f0_other),
            (Respond::AppendEntries(f0_self), Respond::AppendEntries(f0_other)) => f0_self.eq(f0_other),
            (Respond::ReceiveSnapshotChunk(f0_self), Respond::ReceiveSnapshotChunk(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallSnapshot(f0_self), Respond::InstallSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::InstallFullSnapshot(f0_self), Respond::InstallFullSnapshot(f0_other)) => f0_self.eq(f0_other),
            (Respond::Initialize(f0_self), Respond::Initialize(f0_other)) => f0_self.eq(f0_other),
            _unused => false,
        }
    }
}

I am fine with deriving PartialEq for AsyncRuntime. It's also derived for RaftTypeConfig since various structs that use it also implement PartialEq.

Manually tracking enum variant comparisons for PartialEq can introduce bugs if new variants are added without updating the corresponding PartialEq implementation.

[Miaxos]

Reviewable status: 22 of 29 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

And what about this issue? @Miaxos

@schreter quite tricky to answer you haha

I did some work around it to remove the wrapper, but it also introduce some constraint over some types as we can't have OneshotSender<T: !Debug>: Debug when we use a tokio::sync::oneshot::Sender<T> directly. WDYT?

---

openraft/src/engine/command.rs line 260 at r7 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

I am fine with deriving PartialEq for AsyncRuntime. It's also derived for RaftTypeConfig since various structs that use it also implement PartialEq.

Manually tracking enum variant comparisons for PartialEq can introduce bugs if new variants are added without updating the corresponding PartialEq implementation.

Yep, I understand! Done

[Miaxos]

Reviewable status: 22 of 29 files reviewed, 2 unresolved discussions (waiting on @drmingdrmer and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

@schreter quite tricky to answer you haha

I did some work around it to remove the wrapper, but it also introduce some constraint over some types as we can't have OneshotSender<T: !Debug>: Debug when we use a tokio::sync::oneshot::Sender<T> directly. WDYT?

Ci is failing, because did not modify those tests yet, I still believe the wrapper was better as it did not include any additional type constraint. I'm not sure how we can do to avoid the wrapper and do not include additional type constraints.

[schreter]

Reviewed 1 of 9 files at r3, 5 of 9 files at r4, 2 of 3 files at r5, 1 of 4 files at r6, 3 of 3 files at r7, 2 of 5 files at r9, 3 of 3 files at r10, 1 of 2 files at r11, 1 of 2 files at r12, 6 of 6 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @drmingdrmer and @Miaxos)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, Miaxos (Anthony Griffon) wrote…

Ci is failing, because did not modify those tests yet, I still believe the wrapper was better as it did not include any additional type constraint. I'm not sure how we can do to avoid the wrapper and do not include additional type constraints.

Not sure about this issue. In the meantime, I think that the wrapper is the lesser evil, see the other comment.

@drmingdrmer Opinions/ideas?

---

openraft/src/raft/mod.rs line 869 at r13 (raw file):

            + OptionalSend
            + 'static,
        V: OptionalSend + Debug + 'static,

I suppose, this is also due to the lack of channel wrapper, where the channel is only Debug if V: Debug, right?

Code quote:

        V: OptionalSend + Debug + 'static,

[drmingdrmer]

Reviewed 1 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, schreter wrote…

Not sure about this issue. In the meantime, I think that the wrapper is the lesser evil, see the other comment.

@drmingdrmer Opinions/ideas?

@Miaxos you are right. if OneshotSender needs Debug then wrapper is the simplest way.

---

openraft/src/raft/mod.rs line 869 at r13 (raw file):

Previously, schreter wrote…

I suppose, this is also due to the lack of channel wrapper, where the channel is only Debug if V: Debug, right?

This Debug won't be required if we using a wrapper for OneshotSender, right?

[Miaxos]

Reviewable status: 23 of 29 files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @schreter)

---

openraft/src/async_runtime.rs line 103 at r2 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

@Miaxos you are right. if OneshotSender needs Debug then wrapper is the simplest way.

Done

---

openraft/src/raft/mod.rs line 869 at r13 (raw file):

Previously, drmingdrmer (张炎泼) wrote…

This Debug won't be required if we using a wrapper for OneshotSender, right?

It won't be necessary with the wrapper yes.
Done

[drmingdrmer]

Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @schreter)

[schreter]

Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Miaxos)

[drmingdrmer]

Merged! Thank you @Miaxos !