-
Notifications
You must be signed in to change notification settings - Fork 14
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
make TokioSpawner completely stateful #18
base: main
Are you sure you want to change the base?
Conversation
@rmanoka Please, review when you would be able to. From frequent Tokio usage experience, this seems useful |
This looks good, but I am worried about the divergence in impl. across tokio and async-std. The runtime is really just a wrapper because Can you provide some example use-cases where this is relevant? |
I think now So, to summarize, they are almost the same, the only notable difference atm is that async-std doesn't support custom runtime(because, sadly, it isn't capable enough). What I did in case of |
Hello, @rmanoka, just pinging, knowing that messages without mentions are usually missed. I look forward to answering your questions, or providing additional info if needed!! |
Sorry @ftelnov , but I'm not convinced about the use-case of the PR. Currently, I think the impl. is in parity, because internally async's global |
@rmanoka I think both usages are wrong and divergent at some point of view.
You see, they are divergent atm anyway. However, my impl makes them divergent from the different point of view - now tokio spawner assumes that global runtime is available for its methods. Which might be a problem for the end-user. However, that's the problem of those libraries in the end - they use completely different API and idea. This usecase is, indeed, very useful for end-users with tokio runtime. Because use cases, where you have multiple tokio runtimes, are normal. I think, I can lower the divergence between our spawners. Let's bring the same API to the tokio's spawner - in its default implementation, if global runtime is not available yet, we'll create simple one for scheduling, just as you did in |
pub fn block_on<F: Future<Output = T>, T>(future: F) -> T {
LOCAL_EXECUTOR.with(|executor| crate::reactor::block_on(executor.run(future)))
} This seems to be in parity to what we're doing with tokio impl. We could also move to using a thread_local variable in our tokio impl. if it helps anywhere. |
Yes, it would be similar to what I suggested - we'll create runtime by default(if it is not supplied by user). I'll implement it |
Hello, @rmanoka ! I've finished.
Following such logic, we close the gap between async-std and tokio. Note that local runtime is kinda last-resort - as tokio was not designed for such things. |
It closes the gap between tokio and async-std, bringing similar functionality to tokio.
e486217
to
03ec7e3
Compare
Hello, @rmanoka ! Could you please review the recent update I made on this issue? I look forward to providing you any info you might need! |
I propose two things in this PR:
block_on
implementation - as it was spawning brand new runtime every call;AsyncStd
andTokio
spawners(so make themAsyncStdSpawner
andTokioSpawner
) for easier end-user experience.This PR breaks backward-compatibilty by adding suffixes to globally-exposed structures and redesigning
TokioSpawner
.