Proper runtime "enter" instead of spawn + local blocking

[stbuehler]

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

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

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

[yoshuawuyts]

@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]

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]

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]

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

[yoshuawuyts]

@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]

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]

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]

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]

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 rust-lang/futures-rs#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]

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 master...stbuehler:runtime-enter2 for the full idea.

[stbuehler]

Just updated the branch:

• no atomic changes anymore, doesn't touch JoinHandle
• split block_on method into a separate trait BlockingRuntime
  • reduces duplication in the tokio imlementation (as only the block_on implementation differs between Tokio and TokioCurrentThread)
  • the type implementing Runtime can be private now, only the type implementing BlockingRuntime needs to be public - this prevents people abusing entry points in Runtime when it wasn't "entered" before.
  • wasm32 doesn't need a stub implementation for it, it can use a separate trait

[yoshuawuyts]

In the context of #83 (comment) 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!