Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bridging sync/async in "hobby code" where you don't care that much #54

Closed
nikomatsakis opened this issue Mar 18, 2021 · 10 comments · Fixed by #149
Closed

bridging sync/async in "hobby code" where you don't care that much #54

nikomatsakis opened this issue Mar 18, 2021 · 10 comments · Fixed by #149
Labels
good first issue Good for newcomers help wanted Extra attention is needed status-quo-story-ideas "Status quo" user story ideas

Comments

@nikomatsakis
Copy link
Contributor

  • Brief summary:
    • Hacking on a simple client that has to make api calls but where performance isn't much of a priority; many of the libraries are written in an async style, but sometimes you want to make those calls in a location where you can't conveniently make the code async (e.g., an iterator).
    • Currently trying to manage this situation involves either propagating async everywhere or adding block_on calls. But, at least in tokio, block_on calls cannot execute from async threads, so this can lead to panics if that code that is using block_on ever winds up in an async context.
  • Character: Barbara, this was sourced from a conversation with @Mark-Simulacrum

More details from my conversation with @Mark-Simulacrum

  • Context: building things like perf.rust-lang.org, triagebot
  • Performance is not a real issue here, convenience and expressiveness is
    • only have so much time to work on this, trying to stand something up quickly
  • Want a way to make calls to various web services, connect to databases, or do other parts of I/O
  • Would be happy with sync, but the libraries and things are in async
  • Didn't really want to have to pick a runtime, doesn't care much, mostly wants things to Just Work. In the end picked tokio as the only widely known tool at the time, as well as the one that is compatible with things like defaults in hyper.
  • Sometimes find themselves in a synchronous context but need to do an async operation
    • e.g., implementing an iterator
  • Don't care too much about performance, so add a block_on
    • But block-on doesn't take an async block
    • So often nest a spawn inside
    • But then that code gets invoked in an async context, and the code panics
    • Frustrating -- how bad of a problem is it really?
    • Example code
  • Gets in the scenario (example) where
    • something internally has to be make an http request
    • do we make it async?
      • that forces all callers to be async
    • or sync with block-on
      • then cannot be used from an async context
    • or just sync?
      • but then need to find a different http lib (can't use reqwest/hyper)
@nikomatsakis
Copy link
Contributor Author

Lesson IV from this blog post feel pretty related:

https://kevinhoffman.medium.com/rust-async-and-the-terrible-horrible-no-good-very-bad-day-348ebc836274

Lesson IV — Each runtime has a single blocking entry point, so it can only ever block on a single unit of asynchronous work.

...

This one really stumped me. I kept getting errors from the tokio runtime claiming that you can’t create a new runtime from inside another one. I was baffled —”I’m only creating one runtime!”, I shouted at my monitor.

...

It turns out that deep inside the rusoto code there was a function called into_sync_reader() that converted a byte stream future into a synchronous object that implemented the Read trait. I’d used that to get the chunk bytes from S3. As it turns out, the only way to convert asynchronous work into synchronous work is to block on it. So, inside that function, it calls block_on(), which is the tokio runtime’s singleton entry point. For reasons that are only obvious to me now, you can’t block inside a section of code that’s already being blocked on by an outer scope (aka a deadlock). I fixed this problem by mapping the asynchronous stream into a vector of bytes and awaiting the result in an asynchronous fashion that didn’t block or create a new runtime.

@Mark-Simulacrum
Copy link
Member

I think one additional point here that I'm realizing is that fundamentally all of the code when this gets deployed is going to end up being run on a single thread, as commonly I deploy to 1-core VMs on some cloud. That might have interesting implications for the (usually) thread-aware schedulers used, and the extent to which those impose restrictions on my code.

@nikomatsakis
Copy link
Contributor Author

@seanmonstar points out that futures::block_on (if mixed with tokio) will lead to deadlock, not panic

@nikomatsakis
Copy link
Contributor Author

@seanmonstar also pointed me at seanmonstar/reqwest#1215 outlines a pretty similar scenario, except in this case (a) there was a mix of tokio::block_on and futures::block_on, which led to a deadlock, and it was very difficult to track down the root cause.

@pickfire
Copy link
Contributor

pickfire commented Apr 6, 2021

This one really stumped me. I kept getting errors from the tokio runtime claiming that you can’t create a new runtime from inside another one. I was baffled —”I’m only creating one runtime!”, I shouted at my monitor.

From the same blog post, I read that but I didn't know I get to experience the same issue as well, I didn't know it is related until @Darksonn told me so.

So there is a piece of code that works on my machine but not on production, the issue is because my machine have multiple cores and the production server have single core. And a piece of code is blocking the current thread, so the production code didn't work as expected unlike locally, but for my case changing from thread::spawn to task::block_in_place got it working. The code looks something like async -> sync -> async (this part) and I can only modify the last part because all the other parts come from other libraries.

Discussion was here https://discord.com/channels/500028886025895936/747930050674032740/825744899772121139 but other people may have experienced similar issues. Just by looking at #tokio-users discord, we can probably see many issue what others usually faced, or maybe @Darksonn can mention some common cases where people faced.

@nikomatsakis
Copy link
Contributor Author

We wrote a story outline here last Friday but nobody signed up to turn it into prose yet. Anybody interested in doing that? I may take a crack at it, just juggling a lot of things.

@eminence
Copy link
Contributor

eminence commented Apr 6, 2021

I don't know if this helps whomever works on this, but here's some real code that will actually deadlock:

async fn do_web_request(url: &str) -> String {
    reqwest::get(url).await.unwrap().text().await.unwrap()
}

fn aggregate(urls: &[&str]) -> Vec<String> {
    urls
        .iter()
        .map(|url| futures::executor::block_on(do_web_request(url)))
        .collect()
}

#[tokio::main(flavor = "current_thread")]
async fn main() {
    let result = aggregate(&["https://www.rust-lang.org", "https://google.com"]);
    println!("Done!  Have {} results", result.len());
}

And to prove that the block_on stuff is at fault, we can re-write the aggregate function to be async to show that the single-threaded executor can handle this type of concurrency:

async fn aggregate(urls: &[&str]) -> Vec<String> {
    futures::future::join_all(
        urls
            .iter()
            .map(|url| do_web_request(url))
    ).await
}

@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2021

Ah, yes. Using block_on like that will certainly deadlock.

@pickfire
Copy link
Contributor

pickfire commented Apr 7, 2021

@eminence I have amended the docs a bit to add Handle::current. I did tried that in the past since I thought we could reuse the Runtime rather than creating a new Runtime.

fn aggregate(urls: &[&str]) -> Vec<String> {
    let handle = Handle::current();
    urls.iter()
        .map(|url| handle.block_on(do_web_request(url)))
        .collect()
}

But it got a panic as well. But the solution for my case is quite different from yours. There seemed to be many methods to do the same thing but many of them does not work, unlike the sync API the async counterparts is not foolproof enough to be working as long as it compiles.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 7, 2021

The problem is that blocking in async code is just fundamentally problematic. Tokio's own block_on method panics to avoid the deadlock that @eminence got.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed status-quo-story-ideas "Status quo" user story ideas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants