Upgrade from 0.1.2 to 0.1.4 causes hang

[bryanburgers]

A change in 1f2137b caused a hang in our application that did not exist in version 0.1.2.

Here's a reproduction of the issue.

[dependencies]
futures = { version = "0.3", features = ["compat"] }
reqwest = { version = "0.9", default_features = false }
tokio = "0.2.9"
tokio-compat = "=0.1.2"

use futures::compat::Future01CompatExt as _;
use reqwest::r#async::Client;

fn main() {
    tokio_compat::run_std(async {
        futures::executor::block_on(async {
            let client = Client::new();
            let response = client.get("http://www.google.com")
                .send()
                .compat()
                .await;

            match response {
                Ok(response) => println!("status={:?}", response.status()),
                Err(e) => println!("{}", e),
            }
        });
    });

    println!("done.");
}

Using [email protected], this outputs

status=200
done.

Using [email protected], this hangs indefinitely.

---

Notes:

1. I knowingly used an old version of reqwest in this reproduction. Our actual code uses rusoto which doesn't have an async/await-native version yet. Using rusoto in a reproduction is a pain because you need AWS credentials, though.

2. The run_std then block_on seems funny. Our actual code goes through both lambda_runtime and juniper, so run_std and block_on are quite far apart in our actual code, but are still necessary until more of the ecosystem catches up with async/await.

3. If we replace

   tokio_compat::run_std(async {

   with

   let mut runtime = tokio_compat::runtime::Runtime::new().unwrap();
   runtime.block_on_std(async {

   we see the correct and expected behavior in both tokio-compat versions.

[hawkw]

Thanks for the report. I'll take a look!

[hawkw]

This is definitely related to the futures::executor::block_on call. When that's removed, the code no longer hangs. My guess is that because the future is run on the futures::executor executor and not on the tokio-compat runtime, it isn't able to participate in the runtime's idleness tracking, so the runtime fails to shut down. When run_std used block_on, the code is run to completion before we wait for idleness to shut down, so it doesn't keep the runtime running, and reqwest's background task(s?) completing is sufficient for the executor to shut down.

[hawkw]

I am honestly pretty surprised that code that calls futures::executor::block_on inside of a future run by the tokio runtime has ever worked on the compatibility runtime...

[jatsrt]

@hawkw we are surprised too!

[hawkw]

As far as a solution to this issue goes, I'm not really sure what to say. I'd be willing to change run/run_std back to calling block_on rather than spawn to fix this code, but I don't think calling futures::executor::block_on inside of a future running on any tokio runtime is really a good idea.

When executor::block_on is called, the futures crate's LocalPool executor will try to run the passed in future to completion. When that future yields, it will try to run any tasks spawned on the LocalPool instance (e.g., using the LocalSpawner returned by this method), but if there are no such futures, or they are all waiting on something, it essentially blocks the thread. This means that if it's run inside a future on a Tokio runtime, that future will prevent the worker thread from progressing, blocking any tasks spawned using tokio::spawn (such as reqwest's background tasks) that are running on that worker.

If it's absolutely necessary to use futures::executor::block_on in code that also uses the Tokio runtime, for compatibly reasons, I think you want to treat it as a blocking call, and put the block_on inside of tokio::task::block_in_place or tokio::task::spawn_blocking. That way, the block_on call won't block a thread that's trying to run other tasks. I think this should also fix the issue caused by upgrading to tokio-compat 0.1.4 at the same time.

Hopefully that makes sense, and, again, sorry for the breakage!

[bryanburgers]

@hawkw Thanks for looking into this!

I'll be honest. I had glossed over our futures::executor::block_on call many times, and never did it actually occur to me that it was using a different executor. I think I must have assumed it used whatever the current local thread executor was or something, but in retrospect it should have been obvious that it was using a different executor.

I'd be willing to change...

If it's absolutely necessary to use futures::executor::block_on...

I guess at this point, it seems like this isn't really tokio-compat's problem, and it's be pretty presumptuous of me to ask you go backward to fix this issue for us. We already have a workaround in place in production (using tokio_compat::runtime::Runtime::block_on_std). And it seems to be more of an oversight than a necessity that we're using the second executor – or maybe it was a necessity once but no longer is – so sometime in the near future I'll look into replacing our futures::executor::block_on call with one that uses the tokio executor so we don't have two executors running.

Again, thanks for diving in! And thanks for tokio-compat! Like probably many others in the ecosystem right now, we're living in a split ecosystem, and tokio-compat is helping us navigate it just a little bit better while everyone works hard to make progress.

[hawkw]

I had glossed over our futures::executor::block_on call many times, and never did it actually occur to me that it was using a different executor.

Yeah, I think the documentation in the futures crate for this function could really use some work — the behavior of executor::block_oncould probably be a bit more obvious...

I think I must have assumed it used whatever the current local thread executor was or something, but in retrospect it should have been obvious that it was using a different executor.

Yeah, unfortunately, there's no runtime-agnostic concept of a "current executor" — tokio has a notion of a current runtime, async-std has a single global runtime, and the futures crate basically relies on passing around spawn handles (it doesn't have an ambiently-available free function spawn à la tokio/async-std). There isn't currently a way to abstract over these different runtime APIs, which I think would currently require buy-in from all of those executor implementations. Hopefully that changes in the future.

My guess is that you should just be able to remove executor::block_on calls in your code, unless you're relying on them to drive code that spawns on the futures crate's LocalPool. I doubt that's the case, because, as far as I can tell, there isn't a way to spawn on a LocalPool without a LocalSpawner handle, which you don't get when using executor::block_on. So I'd be pretty surprised if taking them out did anything other than making your app run faster :)

Again, thanks for diving in! And thanks for tokio-compat! Like probably many others in the ecosystem right now, we're living in a split ecosystem, and tokio-compat is helping us navigate it just a little bit better while everyone works hard to make progress.

Glad to help! If you have any more issues, please me us know, either by opening a new issue, or on the Tokio Discord server.

[hawkw]

@bryanburgers I'm going to close this issue, since I believe we found a solution. If anything else comes up, please do feel free to open new issues!