runtime: expose the Runtime structure

[jiangliu]

Expose the Runtime structure, so caller could explicitly create a
Runtime object and repeatly invokes block_on() on it.

Signed-off-by: Jiang Liu gerry@linux.alibaba.com

[Noah-Kennedy]

Looking at this, if we are going to do this, we probably want to store the tokio runtime in the Runtime structure, rather than building it anew each time.

[jiangliu]

Looking at this, if we are going to do this, we probably want to store the tokio runtime in the Runtime structure, rather than building it anew each time.

Sorry, I missed your point here.
The Runtime structure already has a field rt: tokio::runtime::Runtime.
Or do you mean to enhance tokio_uring::start() by using a scoped_thread_local(static CURRENT: Runtime)?

diff --git a/src/lib.rs b/src/lib.rs
index 5d675ea..cef1f10 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -140,8 +140,7 @@ use std::future::Future;
 /// }
 /// ```
 pub fn start<F: Future>(future: F) -> F::Output {
-    let mut rt = runtime::Runtime::new().unwrap();
-    rt.block_on(future)
+    runtime::CURRENT_RUNTIME.with(|rt| rt.borrow_mut().block_on(future))
 }
 
 /// A specialized `Result` type for `io-uring` operations with buffers.
diff --git a/src/runtime.rs b/src/runtime.rs
index 3d22bf2..23823de 100644
--- a/src/runtime.rs
+++ b/src/runtime.rs
@@ -6,6 +6,10 @@ use std::io;
 use tokio::io::unix::AsyncFd;
 use tokio::task::LocalSet;
 
+thread_local! {
+    pub(crate) static CURRENT_RUNTIME: RefCell<Runtime> = RefCell::new(Runtime::new().unwrap());
+}
+
 /// The tokio-uring runtime based on the Tokio current thread runtime.
 pub struct Runtime {
     /// io-uring driver

[Noah-Kennedy]

Sorry, I totally forgot about this PR!

I think we should take a step back before we implement this, and get an issue up for this. We should establish first the semantics of the Runtime structure, as well as a builder.

[nik9000]

Hello folks! I came here from trying to use criterion to benchmark a weekend project I was playing with that uses tokio-uring. The criterion folks seem to need to be able to call your block_on method. At least, I think so. I don't really know rust, tokio, or iouring so I may very well be missing a lot here. At least, they need that if they're going to join an existing runtime. If I wanted the benchmark to include starting the runtime and opening files and such then I imagine I could just start a new runtime inside....

[nik9000]

So..... Like I said, I don't know what I'm doing, but I was able to benchmark by setting up in a tokio_uring::start, returning a path, and doing a new tokio_uring::start inside the benchmark. The start and file opening didn't show up in the flamegraph but I presume that's because I shot for something in the millisecond range.

Like I said, all criterion really wants is something it can block_on the future.

[Noah-Kennedy]

@nik9000 I'd agree that we need this. Unfortunately, this PR seems to be dead. Maybe I or @FrankReh can open up another one to get this in, although we should first come up with an issue to scope out what we actually want the behavior to look like.

[nik9000]

although we should first come up with an issue to scope out what we actually want the behavior to look like.

Is #91 ok?

[Noah-Kennedy]

I forgot we had that.

[ollie-etl]

@Noah-Kennedy @FrankReh If I recreate this PR, is there an appetite for merging it? I find myself doing something similar constantly to enable bench marking with criterion.

I actually go further, and change the signature of block_on to be &Self rather than &mut Self

[FrankReh]

Yes, we should make it easy for at least the criterion use.

People using this crate know that APIs can still change right? So we are allowed to do something and then later change our minds about it, at least the API around it?

For example, seeing this issue just today, I realize I don't know much about tokio proper's block_on method and I find there are three. And a very interesting one is a Handle block_on and that the Handle public struct is going through a lot of changes recently but it is a per runtime type and it supports reference counting. So that's something to wonder about. Should this crate be following a similar design pattern? This crate already needs its own block_on method, which wasn't public yet - can it be made as flexible as the tokio version and if not why, and if so, should this crate also offer a Handle that is internally reference counted?

So yes, I think the block_on should be exposed.

@Noah-Kennedy Do you or anyone else want to chime in here? Also I'm wondering about this crate's version and the CHANGELOG.md file. Is there another crate that would make a good example of the kind of care that should go into the change log and when minor or patch numbers are incremented when the project is a major 0 version?

Back to an immediate question, block_on self being &Self or &mut Self? What are the possible pros and cons? Tokio has their's as &Self.

[ollie-etl]

@FrankReh Certianly my view is any crate with version < 1.0.0 is completely free to change API. Its probably a minor version bump, to flag to existing users, although this specific change shouldn't actually affect anyone

[ollie-etl]

@FrankReh I tried reasoning about block_on self being &Self or &mut Self?. We're fixed on the current thread anyway, so parallel acces cannot happen. I guess you could do something odd with tokio_uring running under another executor. My rational is driven entirely by https://bheisler.github.io/criterion.rs/criterion/async_executor/trait.AsyncExecutor.html.

[FrankReh]

@ollie-etl That seems to be reason enough. Any complaints from the compiler when you make the referenced shared?

[ollie-etl]

@FrankReh no, and the benchmarks run quite happily

[FrankReh]

@ollie-etl I assume you've used tokio_uring::start or pulled that apart to get at the block_on method. Have you looked at using the tokio_uring::builder() API which was added a little while back to allow more details about the uring setup to be exposed before calling start? That replaced the global start with a Builder start method. I wonder if that Builder start method should have been named block_on?

I think it's reasonable to make as minimal a change as possible to make the criterion user happy for now. We can go with a 0.3.1 version for it, even if it's not needed. (I don't know what triggers updates past my use of 'cargo update' once in a while.)

Would be nice if the commit message included a short example of a main that used criterion to save newcomers from having to figure that out too. The commit message could also point out that this isn't behind a config flag but we don't know what we don't know yet about future minor bumps to the API.

I'm going to continue my wanderings about this crate's runtime on #91.

[FrankReh]

I believe the issue this PR was addressing has now been handled by #148 so am closing. Thank you for the investigation and in getting the ball rolling.