rmanoka / async-scoped

A scope for async_std and tokio to spawn non-static futures
117 stars 14 forks source link

make TokioSpawner completely stateful #18

Open ftelnov opened 9 months ago

ftelnov commented 9 months ago

I propose two things in this PR:

1) Make tokio spawner completely stateful, so it could take a handle to custom runtime. This is needed, because we don't want the consumers of library to implement the traits themselves. Instead, after this PR, we would be able to simply allow them to use the default implementation. By default tokio spawner would take global runtime. It also fixes tokio's block_on implementation - as it was spawning brand new runtime every call; 2) Add suffixes to AsyncStd and Tokio spawners(so make them AsyncStdSpawner and TokioSpawner) for easier end-user experience.

This PR breaks backward-compatibilty by adding suffixes to globally-exposed structures and redesigning TokioSpawner.

ftelnov commented 9 months ago

@rmanoka Please, review when you would be able to. From frequent Tokio usage experience, this seems useful

rmanoka commented 9 months ago

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 tokio_task::block_on consumes a function, not a future.

Can you provide some example use-cases where this is relevant?

ftelnov commented 9 months ago

I think now AsyncStd and Tokio spawners share the same implementation. Previously, you created runtime in tokio's block_on - even if it is just for wrapping, I guess some work would be done anyway. However, async-std spawner doesn't do such work - it simply uses global executor on every function.

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 block_on is I just removed the creation of new runtime.

ftelnov commented 9 months ago

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!!

rmanoka commented 9 months ago

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 block_on does something like creating a Task and scheduling it on the local thread. We are doing the same in tokio. Storing the executor that is meant to just run the particular future doesn't seem very useful to me.

ftelnov commented 9 months ago

@rmanoka I think both usages are wrong and divergent at some point of view. Currently: 1) async-std spawner just creates the task and delegates its completion to the current globally available runtime. It ensures itself whether it is already created. Runtime is not being erased after block is finished, so block_on basically just uses some global entity to be ran on; 2) tokio spawner creates the brand new runtime, schedules the future on it, blocks the current thread and then runtime is gone.

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. async-std is not really a runtime, it is an async version of the STD. It is just us who use it the same way as tokio.

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 block_on. It will both close the gap between runtimes and provide the same semantics for the end-user. What do you think?

rmanoka commented 9 months ago

async-std uses this line that uses:

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.

ftelnov commented 9 months ago

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

ftelnov commented 9 months ago

Hello, @rmanoka ! I've finished. Currently I use such logic for tokio: 1) If global runtime is available, we'll block on it; 2) If global runtime is not available, create ad-hoc local-thread runtime, use it; 3) When spawner is done, its ad-hoc runtime is being freed.

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.

ftelnov commented 7 months ago

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!