search

rustasync / runtime

Empowering everyone to build asynchronous software
https://docs.rs/runtime
Apache License 2.0
862 stars 28 forks source link

Proper runtime "enter" instead of spawn + local blocking #73

Closed stbuehler closed 5 years ago

stbuehler commented 5 years ago

Remove the necessity to spawn threads by using an explicit enter method in the Runtime trait which blocks on the passed future.

Description, Motivation and Context

Currently all runtimes need to run in background threads / worker pools.

With this PR this is changed: there is an explicit enter which is supposed to block on the passed future, so a current_thread runtime doesn't need to spawn new threads (which makes strace way easier to read, and is simply cleaner imho).

This also removes various (lazy_statics) globals: if a runtime is entered again, it will simply have to recreate the runtime environment, and it should shutdown cleanly on leaving enter. (One might argue whether it should only block on the passed future or on a more generic "runtime shutdown".)

The second commit avoids double-boxing with a more generic JoinHandle and its type-erased CompletionHandle part.

This also makes it more explicit that enter can't work on wasm; I went for a simple panic though instead of going #[cfg(...)]-crazy over the API (especially as the existing wasm cfg handling is broken and looks wrong).

Types of changes

(Although the breaking change should only be visible for runtime_raw users.)

yoshuawuyts commented 5 years ago

@stbuehler Hey there! Overall I like this idea a lot! It'd be nice if it the method could be named block_on to match the futures lib. Would you mind changing it to that? Thanks heaps!

Also cc/ @stjepang, there's a bunch of atomic operations going on that I don't feel confident in reviewing. Would you mind taking a look? Thanks!

stbuehler commented 5 years ago

I'm certainly open to renaming it - my initial choice was indeed block_on :) But may I suggest having an additional word in it to make it clear it is more than just block_on - maybe start_and_block_on (my favorite choice right now) or enter_and_block_on (to link it to runtime_raw::enter)?

If we go with start_and_block_on one might add a fn start(&self) method later for platforms like wasm which run forever, can't block and where you can only have (and start) one runtime (for example like this: https://github.com/stbuehler/runtime/commit/611231ab57dcdbd6e0929b5dafa4e8819924659e).

I'll have a look at the atomics again too; the synchronization of data with completed should be fine (Acquire/Release are basically made for this exact use case) - but I'm not sure AtomicWaker synchronizes properly (it might need a full barrier), although I think that should count as bug in AtomicWaker...

stbuehler commented 5 years ago

I took a look at the source of futures_util::future::abortable and they only use Relaxed to set and check the flag (as AtomicWaker uses AcqRel operations internally providing a full barrier), so the atomic ordering should be fine.

stbuehler commented 5 years ago

Renamed enter to start_and_block_on, and remembered how to spawn on "currently running executor (per thread)" in tokio, removing some complexity.

yoshuawuyts commented 5 years ago

@stbuehler dang yeah, the fact that the WASM spawn function doesn't block is a bit inconvenient. I do really like short names, so I'd prefer runtime::enter over any longer variants (and in turn the blocking behavior can vary per program).

Still, I kind of wish we could have a WASM futures executor function with the same semantics as the other block_on functions. Perhaps we should open an issue about this.

stbuehler commented 5 years ago

So you want me to rename it back to enter (I don't mind doing it, just want to make sure :) )?

(In case you didn't notice: I'm also trying to get a RemoteCompletionHandle into futures-rs, so RemoteHandle from futures-rs would be able to replace JoinHandle, solving the atomic ordering issue here.)

yoshuawuyts commented 5 years ago

After reading the PR again, I noticed that the wasm32 implementation simply panics. This seems like it's indeed the right model. That also means that calling it block_on is in fact an accurate name, and would be preferable over any other.

I still don't feel qualified to review any of the atomics code, pinging @stjepang again for a review pass.

ghost commented 5 years ago

I'd kind of prefer holding off merging this change as it increases complexity quite a bit for comparatively little benefit, which is essentially starting one thread less. I don't feel comfortable saying this is the right step forward as there are many unknowns here. Maybe we can revisit this PR later at some point?

stbuehler commented 5 years ago

I'd kind of prefer holding off merging this change as it increases complexity quite a bit for comparatively little benefit, which is essentially starting one thread less. I don't feel comfortable saying this is the right step forward as there are many unknowns here. Maybe we can revisit this PR later at some point?

I think the first commit decreases complexity and improves the API significantly, and have no interest in waiting for it.

The second commit increases the complexity of the JoinHandle, yes; I don't mind removing that part (it only costs some boxes for the initial future to be entered), and hope that https://github.com/rust-lang-nursery/futures-rs/pull/1766 goes through - then a later change could use RemoteCompletionHandle from futures. Also if we drop the second commit I think the current JoinHandle could already be removed in favor of RemoteHandle.

stbuehler commented 5 years ago

How about this?

/// Runtime trait for runtimes supporting blocking.
pub trait BlockingRuntime<F, T>
where
    F: Future<Output = T>,
{
    /// Runs a future inside the runtime and blocks on the result.
    ///
    /// Needs to call `enter_runtime` or `set_runtime` (only on background threads) in threads
    /// running futures.
    fn block_on(&self, fut: F) -> T;
}

This would remove the need for boxing, support !Send if the runtime supports it, and no atomic memory ordering to review.

See https://github.com/rustasync/runtime/compare/master...stbuehler:runtime-enter2 for the full idea.

stbuehler commented 5 years ago

Just updated the branch:

yoshuawuyts commented 5 years ago

In the context of https://github.com/rustasync/runtime/pull/83#issuecomment-520150680 I'm going to go ahead and close this PR. The way our interactions have been going recently has not been great, and I think it's in everyone's best interest if we part ways here. Thanks for your contributions, and good luck!