Never create more than one Tokio runtime. Remove tokio_util::block_on

[ry]

There is a hack we've introduced called tokio_util::block_on, which can execute a future in a new Tokio runtime. Not only is this blocking, but it creates many extra threads. We use it in the module loader to avoid certain deadlocks - fundamentally the things we are blocking on are simple network connections which could be done on a single thread.

Recently we parallelized some module loading, which caused a visible explosion in the number of threads.

This tokio_util::block_on is a problem of our own creation (indeed, it's my design mistake). There is no fundamental reason for it to exist. We should be able to load and execute complex code on a single thread. It may be some of our fundamental abstractions are causing this even, and need to be reconsidered. Namely the idea that an Isolate is a Future and that all of its ops are polled manually from that future.

This is a v1.0 blocker.

[hayd]

Is the issue we should be using .await instead block_on (and rewrite higher up the stack in terms of futures), or is there something fundamental I'm missing ?

[ry]

@hayd No - it's more complicated than that... I'll try to write up something later. But it has to do with how Isolate::poll is designed.

[bartlomieju]

This maybe a naive question, but why didn't we use tokio_threadpool::blocking in the first place? It seems like exactly the thing that is needed for this kind of job - won't spawn new thread and can be used for synchronous code (it's even used in JSON dispatch now for sync calls).

EDIT: Nevermind... tokio_threadpool::blocking doesn't convert async code to sync. futures::Future::wait is the closest to what we need but it doesn't work in this case.

[ry]

Update: in #3043 the major bottleneck to removing tokio_util::block_on was eliminated by reorganizing how the TS compiler works.

There are still many usages:

> grep block_on cli -R
cli/http_util.rs:    tokio_util::block_on(fetch_string_once(url))
cli/worker.rs:    tokio_util::block_on(self.execute_mod_async(module_specifier, is_prefetch))
cli/file_fetcher.rs:    tokio_util::block_on(self.fetch_source_file_async(specifier))
cli/file_fetcher.rs:      tokio_util::block_on(self.fetch_remote_source_async(
cli/file_fetcher.rs:      tokio_util::block_on(self.get_source_file_async(
cli/file_fetcher.rs:      let result = tokio_util::block_on(fetcher.fetch_remote_source_async(
cli/tokio_util.rs:  rt.block_on_all(future).unwrap();
cli/tokio_util.rs:pub fn block_on<F, R>(future: F) -> Result<R, ErrBox>
cli/tokio_util.rs:      Ok(mut rt) => rt.block_on(future),
cli/ops/dispatch_minimal.rs:      // tokio_util::block_on.
cli/compilers/ts.rs:      tokio_util::block_on(self.compile_async(state, source_file))
cli/compilers/ts.rs:    assert!(tokio_util::block_on(out).is_ok());

but I suspect these should be easily removable now...

[bartlomieju]

@ry most of those usages are for test. I'll look into it

[bartlomieju]

Update: with #3059 and #3080 landed, we're left with a single call to tokio_util::block_on in SourceFileFetcher::fetch_source_file. Also we don't create a new threadpool runtime for this purpose but rather use tokio::runtime::current_thread effectively meaning that running this blocking section requires a new thread.