-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a Monoio async runtime #1010
base: main
Are you sure you want to change the base?
Conversation
ba455eb
to
37a921e
Compare
37a921e
to
09a4e46
Compare
09a4e46
to
8b7e34b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that a new runtime is now being added :-).
BTW, we should probably make all required types part of the runtime, so tokio
would be optional as well. Currently, oneshot and mpsc channels as well as some other primitives are taken 1:1 from Tokio.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Miaxos)
openraft/src/async_runtime.rs
line 220 at r1 (raw file):
// // We would need to transmit a `[monoio::io::Canceller]` to the future we want to // spawn.
Feel free to propose an API where you can pass the Canceller
around properly. Can't it be part of a JoinHandle
(i.e., wrap monoio::JoinHandle
in a new type)?
Code quote:
// No abort in monoio task, it's a little complicated, as it's using io-uring when
// available, you register for update from the kernel, but when a task is dropped, you
// need to "cancel" this registration to the kernel too.
//
// We would need to transmit a `[monoio::io::Canceller]` to the future we want to
// spawn.
I do completely agree!
👍
Yeah, well, you need to have access to the Canceller when you want to abort the task, but you'll also need to have access to the Canceller when you do some IO related to io-uring when you are inside the spawned task, so you need it when you are spawning your task, something like this could be possible: #[inline]
fn spawn<T>(fn: F) -> Self::JoinHandle<T::Output>
where
T: Future + OptionalSend + 'static,
T::Output: OptionalSend + 'static,
F: FnOnce(&AsyncRuntimeContext) -> T,
{
...
} I'll try to have some ideas around, and it would be better to also have integration tests running on this new runtime too (linked to drmingdrmer/async-entry#1 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter)
openraft/src/async_runtime.rs
line 220 at r1 (raw file):
Previously, schreter wrote…
Feel free to propose an API where you can pass the
Canceller
around properly. Can't it be part of aJoinHandle
(i.e., wrapmonoio::JoinHandle
in a new type)?
It appears infeasible to integrate monoio and tokio given that monoio's
JoinHandle
lacks an abort()
method.
And only specific futures(created with monoio::io::CancelableAsyncReadRent
) in
monoio can be canceled, the others can not.
As a potential solution for Openraft, removing AsyncRuntime::abort()
might be
a viable option since its primary use is for terminating the Timer
. Instead of
relying on abort()
, the Timer
can be equipped with a shutdown mechanism
utilizing a oneshot::channel()
to issue a termination signal, thus gracefully
ceasing its operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, we should probably make all required types part of the runtime, so
tokio
would be optional as well. Currently, oneshot and mpsc channels as well as some other primitives are taken 1:1 from Tokio.
@schreter
Is there a reason to abstract tokio::sync::*
primitives when they can be directly used in asynchronous runtimes other than Tokio?
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter)
Not all of the async-runtime provide an `abort()` primitive, such as [`monoio`](https://crates.io/crates/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: - datafuselabs#1010 (review) - The above discussion is part of datafuselabs#1010
Not all of the async-runtime provide an `abort()` primitive, such as [`monoio`](https://crates.io/crates/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: - datafuselabs#1010 (review) - The above discussion is part of datafuselabs#1010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to abstract
tokio::sync::*
primitives when they can be directly used in asynchronous runtimes other than Tokio?
Yes. Tokio primitives do work in other runtimes, but they are not optimally-supported.
For example, Tokio has its own counters for cooperative multitasking (which, obviously, only make sense and are active if running within Tokio), other runtimes have their own algorithm for how not to block the runtime for too long. The same is true for resource monitoring support (e.g., via tracing
in Tokio).
Another issue is that Tokio sync primitives allocate memory at unexpected points. This is OK if you are fine with your program going broken on OOM, but we don't have this luxury :-). We need to handle OOMs properly.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer)
openraft/src/async_runtime.rs
line 220 at r1 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
It appears infeasible to integrate monoio and tokio given that monoio's
JoinHandle
lacks anabort()
method.And only specific futures(created with
monoio::io::CancelableAsyncReadRent
) in
monoio can be canceled, the others can not.As a potential solution for Openraft, removing
AsyncRuntime::abort()
might be
a viable option since its primary use is for terminating theTimer
. Instead of
relying onabort()
, theTimer
can be equipped with a shutdown mechanism
utilizing aoneshot::channel()
to issue a termination signal, thus gracefully
ceasing its operation.
+1
Not all of the async-runtime provide an `abort()` primitive, such as [`monoio`](https://crates.io/crates/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: - #1010 (review) - The above discussion is part of #1010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monoio JoinHandle does not return a Result<T,E>
but just T
,
Reviewed 4 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)
memstore/src/lib.rs
line 412 at r1 (raw file):
#[tracing::instrument(level = "trace", skip(self, entries))] async fn append_to_log<I>(&mut self, entries: I) -> Result<(), StorageError<MemNodeId>> where I: IntoIterator<Item = Entry<TypeConfig>> + Send {
Does feature flag monoio
implies singlethreaded
?
If singlethreaded
is enabled for Openraft, Openraft does require Send
bound for types.
If it is, monoio
feature flag should be declared as monoio = ["singlethreaded"]
.
And memstore
does not use Send
bound correctly. It should use openraft::OptionalSend
here.
Let me fix this issue first.
openraft/src/async_runtime.rs
line 220 at r1 (raw file):
Previously, schreter wrote…
+1
abort()
is removed, life become easier:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Miaxos and @schreter)
memstore/src/lib.rs
line 412 at r1 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
Does feature flag
monoio
impliessinglethreaded
?
Ifsinglethreaded
is enabled for Openraft, Openraft does requireSend
bound for types.If it is,
monoio
feature flag should be declared asmonoio = ["singlethreaded"]
.And
memstore
does not useSend
bound correctly. It should useopenraft::OptionalSend
here.Let me fix this issue first.
Oh I see that this PR already specified monoio = ["singlethreaded"]
.
414e1b5
to
de2aa06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 30 files at r2, all commit messages.
Reviewable status: 16 of 30 files reviewed, 4 unresolved discussions (waiting on @Miaxos and @schreter)
openraft/src/async_runtime.rs
line 138 at r2 (raw file):
#[cfg(feature = "monoio")] pub mod monoio {
This file may expand soon if more runtimes are added.
It would be better to promote async_runtime.rs
to a mod dir and put every runtime implementation in a separate file.
tests/tests/append_entries/t10_see_higher_vote.rs
line 72 at r2 (raw file):
let n0 = router.get_raft_handle(&0)?; spawn(async move {
It can be just replaced with <TypeConfig as RaftTypeConfig\>::AsyncRuntime::spawn
.
tests/tests/append_entries/t11_append_conflicts.rs
line 234 at r2 (raw file):
Err(err) => { dbg!(err); anyhow::bail!("blbl")
@@ -194,6 +196,7 @@ async fn add_learner_with_set_nodes() -> Result<()> { | |||
/// Because adding learner is also a change-membership operation, a new membership config log will | |||
/// let raft consider the previous membership config log as committed, which is actually not. | |||
#[async_entry::test(worker_threads = 8, init = "init_default_ut_tracing()", tracing_span = "debug")] | |||
#[cfg_attr(feature = "monoio", ignore)] // Crashing the future is causing a whole crash with monoio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix
@@ -211,13 +214,13 @@ async fn add_learner_when_previous_membership_not_committed() -> Result<()> { | |||
router.set_network_error(1, true); | |||
|
|||
let node = router.get_raft_handle(&0)?; | |||
tokio::spawn(async move { | |||
<TypeConfig as RaftTypeConfig>::AsyncRuntime::spawn(async move { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using it seems to not be a good idea for those tests, at the end we have spawn
and spawn_local
, when inside a test we could have openraft
running with singlethread
so each spawn has to be spawn_local
but the test is running with multi-threading, so for tests we need spawn
. Need to revisit this, I'll revert back to have some spawn
abstracted over the feature flag. (wdyt @drmingdrmer @schreter ?)
At the end, I'm wondering, when we should & when we shouldn't use AsyncRuntime
implementation in those tests. (like spawn
here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad, it's not happening when testing with tokio right now due to the fact we never test singlethread
with tokio, it was happening on my local test due to a misconfiguration on my end. (I enabled the monoio
default feature flag, so it was singlethread
even when testing against tokio 😇 )
Still, I would love to hear what you think about this.
d254005
to
cf7f71e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 30 files at r2, 19 of 26 files at r3, 1 of 2 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @drmingdrmer and @Miaxos)
README.md
line 4 at r5 (raw file):
<h1>Openraft</h1> <h4> Advanced <a href="https://raft.github.io/">Raft</a> in 🦀 Rust using <a href="https://tokio.rs/">Tokio</a> or <a href="https://github.com/bytedance/monoio/">Monoio</a>. Please ⭐ on <a href="https://github.com/datafuselabs/openraft">github</a>!
Split long lines?
openraft/src/lib.rs
line 81 at r5 (raw file):
#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioRuntime; pub use crate::async_runtime::tokio::TokioInstant; pub use crate::async_runtime::tokio::TokioRuntime;
?
BTW do we need MonoioInstant
/TokioInstant
at all?
Suggestion:
#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioInstant;
#[cfg(feature = "monoio")] pub use crate::async_runtime::monoio::MonoioRuntime;
#[cfg(not(feature = "monoio"))] pub use crate::async_runtime::tokio::TokioInstant;
#[cfg(not(feature = "monoio"))] pub use crate::async_runtime::tokio::TokioRuntime;
openraft/src/async_runtime/mod.rs
line 10 at r5 (raw file):
use crate::OptionalSync; pub mod tokio;
Same here - only include if relevant?
Suggestion:
#[cfg(not(feature = "monoio"))] pub mod tokio;
tests/tests/fixtures/runtime.rs
line 6 at r5 (raw file):
#[cfg(feature = "monoio")] pub use monoio::spawn; #[cfg(not(feature = "monoio"))] pub use tokio::spawn; #[cfg(not(feature = "monoio"))] pub use tokio::sync::oneshot;
Shouldn't these be simply part of the AsyncRuntime
and used from there?
Code quote:
#[cfg(feature = "monoio")] pub use local_sync::oneshot;
#[cfg(feature = "monoio")] pub use monoio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::spawn;
#[cfg(not(feature = "monoio"))] pub use tokio::sync::oneshot;
tests/tests/membership/t11_add_learner.rs
line 217 at r3 (raw file):
Previously, Miaxos (Anthony Griffon) wrote…
Oh, my bad, it's not happening when testing with tokio right now due to the fact we never test
singlethread
with tokio, it was happening on my local test due to a misconfiguration on my end. (I enabled themonoio
default feature flag, so it wassinglethread
even when testing against tokio 😇 )Still, I would love to hear what you think about this.
I think we don't need to differentiate between spawn
/spawn_local
. The AsyncRuntime
exposes the "right" spawn
based on the type of the runtime. I.e., always use spawn
from RaftTypeConfig::AsyncRuntime
.
The singlethreaded
feature was originally built for our proprietary runtime, which is kind of similar to monoio
in the sense that we have thread-per-core and use !Send
futures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Miaxos and @schreter)
tests/tests/membership/t11_add_learner.rs
line 217 at r3 (raw file):
Previously, schreter wrote…
I think we don't need to differentiate between
spawn
/spawn_local
. TheAsyncRuntime
exposes the "right"spawn
based on the type of the runtime. I.e., always usespawn
fromRaftTypeConfig::AsyncRuntime
.The
singlethreaded
feature was originally built for our proprietary runtime, which is kind of similar tomonoio
in the sense that we have thread-per-core and use!Send
futures.
+1
cf7f71e
to
90b3c5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 22 of 34 files reviewed, 9 unresolved discussions (waiting on @drmingdrmer and @schreter)
tests/tests/append_entries/t10_see_higher_vote.rs
line 72 at r2 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
It can be just replaced with
<TypeConfig as RaftTypeConfig\>::AsyncRuntime::spawn
.
Done.
tests/tests/append_entries/t11_append_conflicts.rs
line 234 at r2 (raw file):
Err(err) => { dbg!(err); anyhow::bail!("blbl")
Done.
tests/tests/fixtures/runtime.rs
line 6 at r5 (raw file):
Previously, schreter wrote…
Shouldn't these be simply part of the
AsyncRuntime
and used from there?
Yep, but I was thinking of doing it in another PR linked to #1013 (the spawn will be removed before that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 26 files at r3, 10 of 12 files at r6, 1 of 3 files at r7, all commit messages.
Reviewable status: 31 of 34 files reviewed, 7 unresolved discussions (waiting on @Miaxos and @schreter)
tests/tests/membership/t11_add_learner.rs
line 199 at r3 (raw file):
Previously, Miaxos (Anthony Griffon) wrote…
To fix
Is this resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 31 of 34 files reviewed, 7 unresolved discussions (waiting on @drmingdrmer and @schreter)
tests/tests/membership/t11_add_learner.rs
line 199 at r3 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
Is this resolved?
Not yet, it's not a crash but it seems there is an issue with preemption & monoio, basically the main loop is executing but the following tasks are not being taken by the runtime. I'm looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 31 of 35 files reviewed, 7 unresolved discussions (waiting on @drmingdrmer and @schreter)
tests/tests/membership/t11_add_learner.rs
line 199 at r3 (raw file):
Previously, Miaxos (Anthony Griffon) wrote…
Not yet, it's not a crash but it seems there is an issue with preemption & monoio, basically the main loop is executing but the following tasks are not being taken by the runtime. I'm looking into it.
This ab51c74 should solve this issue but the main issue belong to the runtime at then end. Going to try to have a reproducible example to submit to monoio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 31 of 35 files reviewed, 7 unresolved discussions (waiting on @drmingdrmer and @schreter)
tests/tests/membership/t11_add_learner.rs
line 199 at r3 (raw file):
Previously, Miaxos (Anthony Griffon) wrote…
This ab51c74 should solve this issue but the main issue belong to the runtime at then end. Going to try to have a reproducible example to submit to monoio.
From the tests I'm doing with monoio
, it seems the issue is linked to the fact we are using oneshot
channels from tokio
with monoio
which cause this issue at the end. I should be able to solve it without the hack by using a specific oneshot
channel which wouldn't be the one from tokio
. #1026
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
…eaded tests (mb fixup) Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
… runtime Signed-off-by: Anthony Griffon <anthony@griffon.one>
ab51c74
to
f9cad89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r8, 5 of 9 files at r9, 3 of 3 files at r11, all commit messages.
Reviewable status: 32 of 36 files reviewed, 8 unresolved discussions (waiting on @Miaxos and @schreter)
openraft/src/lib.rs
line 81 at r5 (raw file):
Previously, schreter wrote…
?
BTW do we need
MonoioInstant
/TokioInstant
at all?
I do not get it either why to import MonoioInstant/MonoioRuntime. 🤔
openraft/src/async_runtime/mod.rs
line 10 at r11 (raw file):
use crate::OptionalSync; #[cfg(not(feature = "monoio"))] pub mod tokio;
Shall we add a feature tokio
and make it enabled by default?
There will be more AsyncRuntimeprovided in future. Then negative feature assertion like
#[cfg(not(feature = "monoio"))]` does not support more than two runtimes.
BTW what about name runtime related feature flags in form of rt-monoio
and rt-tokio
?
So that developers can find out all runtime related flags quickly.
openraft/src/core/raft_core.rs
line 932 at r11 (raw file):
// an issue #[cfg(feature = "monoio")] monoio::time::sleep(Duration::from_millis(0)).await;
It is the issue with using tokio channel inside monoio runtime, right?
tests/tests/append_entries/t11_append_conflicts.rs
line 229 at r9 (raw file):
C: RaftTypeConfig, LS: RaftLogStorage<C>, C::NodeId: Sync + Send,
Why does it need Sync+Send
bound? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 26 files at r3.
Reviewable status: 32 of 36 files reviewed, 9 unresolved discussions (waiting on @Miaxos and @schreter)
tests/tests/life_cycle/t11_shutdown.rs
line 41 at r11 (raw file):
/// A panicked RaftCore should also return a proper error the next time accessing the `Raft`. #[async_entry::test(worker_threads = 8, init = "init_default_ut_tracing()", tracing_span = "debug")] #[cfg_attr(feature = "monoio", ignore)]
Suggestion:
// monoio `JoinHandle` does not return an `Err` when there is a panic.
#[cfg_attr(feature = "monoio", ignore)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 12 files at r6, 3 of 3 files at r7, 1 of 2 files at r8, 6 of 9 files at r9, 1 of 2 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @drmingdrmer and @Miaxos)
openraft/src/lib.rs
line 81 at r5 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
I do not get it either why to import MonoioInstant/MonoioRuntime. 🤔
Then I'd suggest removing the two imports and check where it fails to compile. Those locations then need to use async runtime from the config.
openraft/src/async_runtime/mod.rs
line 10 at r11 (raw file):
Previously, drmingdrmer (张炎泼) wrote…
Shall we add a feature
tokio
and make it enabled by default?
There will be more AsyncRuntimeprovided in future. Then negative feature assertion like
#[cfg(not(feature = "monoio"))]` does not support more than two runtimes.BTW what about name runtime related feature flags in form of
rt-monoio
andrt-tokio
?
So that developers can find out all runtime related flags quickly.
Yes, good ideas.
openraft/src/async_runtime/monoio.rs
line 92 at r11 (raw file):
impl<T: OptionalSend> Debug for MonoioOneshotSender<T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_tuple("MonoioSendWrapper").finish()
Suggestion:
f.debug_tuple("MonoioOneshotSender").finish()
openraft/src/async_runtime/tokio.rs
line 100 at r11 (raw file):
impl<T: OptionalSend> Debug for TokioOneShotSender<T> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_tuple("TokioSendWrapper").finish()
BTW, shouldn't it be TokioOneshotSender
to harmonize with other names (i.e., lowercase shot
)?
Suggestion:
f.debug_tuple("TokioOneShotSender").finish()
Starting a work around
AsyncRuntime
to have aMonoioRuntime
available & tested.The main issue is around
is_panic
andabort
right now, as to properlyabort
a task by usingmonoio
, you need to cancel the IO with aCanceller
.monoio
available and this PR is also used to find out what steps are needed to achieve this.Checklist
Considerations
worker_threads
have no impact onmonoio
with the current implementation ofasync-entry
References
This change is