refactor: remove calls to tokio_util::block_on

[bartlomieju]

Removes as many calls to tokio_util::block_on as possible

[bartlomieju]

So this task has proven to be a worthy oponent. Writing this down to get other peoples opinions as my knowledge of futures and Tokio runtime comes lacking.

Currently I'm stuck with a few more calls to block_on, fortunately all of them are for tests only. The underlying problem in my opinion might be HTTP client. If you try to .wait() on a future that does HTTP call (eg. cli/file_fetcher.rs tests) then it all breaks. I suspect it might be caused by the fact that reqwest creates a client with default connector which uses 4 threads for DNS resolving.

Here's output from one of failing test:

---- file_fetcher::tests::test_get_source_code_multiple_downloads_of_same_file stdout ----
tools/http_server.py starting...
tools/http_server.py ready
DEBUG RS - deno_cli::file_fetcher:120 - fetch_source_file. specifier http://localhost:4545/tests/subdir/mismatch_ext.ts
DEBUG RS - hyper::client::connect::http:501 - connecting to [::1]:4545
DEBUG RS - tokio_reactor:680 - adding I/O source: 0
DEBUG RS - tokio_reactor::registration:518 - scheduling Write for: 0
DEBUG RS - tokio_reactor:698 - dropping I/O source: 0
thread 'file_fetcher::tests::test_get_source_code_multiple_downloads_of_same_file' panicked at 'assertion failed: result.is_ok()', cli/file_fetcher.rs:896:7

The test fails with following error:

Err(ErrBox(Error(Hyper(Error(Connect, Os { code: 61, kind: ConnectionRefused, message: "Connection refused" })), "http://localhost:4545/tests/subdir/mismatch_ext.ts")))

Even though the server is working and accepting connections above error is returned. In turn it causes PoisonError on test_util::http_server because it's not properly dropped after the panic so other tests start failing.

CC @ry @piscisaureus @kevinkassimo @afinch7 @95th

EDIT: I also tried to rewrite the tests to use async functions and run futures via tokio_util::run but it still doesn't play nicely with test http server.

[ry]

@bartlomieju In the tests that you're getting this error, are we using the single or multithread runtime? Maybe if it's a single thread runtime, wait() blocks everything.

Is it possible to modify this PR so it's just removing some of the block_on calls, and leave the troublesome ones?

[bartlomieju]

@bartlomieju In the tests that you're getting this error, are we using the single or multithread runtime? Maybe if it's a single thread runtime, wait() blocks everything.

We're using "default" runtime which has as many threads as there's CPUs. I tweaked it manually but no success.

Is it possible to modify this PR so it's just removing some of the block_on calls, and leave the troublesome ones?

Yes, trying to get CI green

[ry]

Not sure I counted correctly but this seems to remove as many block_on calls as it adds. I would land anything that’s nets a removal of call sites.

[bartlomieju]

@ry I'm still tinkering with it... I'm not sure that can be solved some other way around. Maybe we could minimize the impact of tokio_util::block_on by adding a small lazy runtime that wouldn't be destroyed after every operation to minimize effects of spawning a regular full fledged thread pool runtime. WDYT?

[ry]

Maybe just wait until we have a proper fix.

[bartlomieju]

Maybe just wait until we have a proper fix.

I give up for now, let's do that...

[bartlomieju]

@ry after all I rewritten some of the tests to remove tokio_util::block_on and instead just run futures. That way I can go down to single usage of block_on in FileFetcher::fetch_source_file. If that sounds good I can rewrite tests in file_fetcher

[ry]

I'm happy as long as there is a net removal of block_on calls... which it seems there is:

> curl -sL https://github.com/denoland/deno/pull/3059.patch | grep block_on | deno xeval 'if (sum == undefined) var sum = 0; if ($[0] == "+") sum++; if ($[0] == "-") sum--; console.log("sum", sum)' | tail -1
sum -1

[bartlomieju]

$ curl -sL https://github.com/denoland/deno/pull/3059.patch | grep block_on
Subject: [PATCH] remove as many calls to tokio_util::block_on as possible
-      tokio_util::block_on(self.compile_async(state, source_file))
-    assert!(tokio_util::block_on(out).is_ok());
-      tokio_util::block_on(self.fetch_remote_source_async(
-      tokio_util::block_on(self.get_source_file_async(
-      let result = tokio_util::block_on(fetcher.fetch_remote_source_async(
-    tokio_util::block_on(fetch_string_once(url))
-    tokio_util::block_on(self.execute_mod_async(module_specifier, is_prefetch))

Effectively we're left with only one call:

//cli/file_fetcher.rs

  ...
  /// Required for TS compiler.
  pub fn fetch_source_file(
    self: &Self,
    specifier: &ModuleSpecifier,
  ) -> Result<SourceFile, ErrBox> {
    tokio_util::block_on(self.fetch_source_file_async(specifier))
  }
  ...

EDIT:
In turn fetch_source_file is used in two places:

• op_cache - op issued by TS compiler, I think we could make it async - it will still error compiler process
• TsCompiler::get_source_line - used for source maps, not sure if we can make it async

[ry]

LGTM - nice clean up - we're almost there!