Skip to content

support tokio async runtimes #11

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

Closed
monaka opened this issue Oct 25, 2022 · 20 comments
Closed

support tokio async runtimes #11

monaka opened this issue Oct 25, 2022 · 20 comments

Comments

@monaka
Copy link
Contributor

monaka commented Oct 25, 2022

Hello,

I'm working to create a wallet-backend.
And I'm getting one test failure.
All other tests are passed.
I suppose it is required to call a shutdown code after I call Wallet::new().
How can I shut down my wallet safely?

test wallet::tests::test_get ... FAILED

failures:

---- wallet::tests::test_get stdout ----
thread 'wallet::tests::test_get' panicked at 'Cannot drop a runtime in a context where blocking is not allowed. This happens when a runtime is dropped from within an asynchronous context.', /home/monaka/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/blocking/shutdown.rs:51:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@monaka
Copy link
Contributor Author

monaka commented Oct 25, 2022

@monaka
Copy link
Contributor Author

monaka commented Oct 27, 2022

I'm not sure but this issue may be caused by using blocking APIs in ConsignmentProxy.
I'm going to create patches.

@zoedberg
Copy link
Member

Calling shutdown should not be necessary after calling new. Sometimes tests can hang if an expect/unwrap/unreachable is encountered during test execution, but that should mean that something is wrong (code or test). Currently there is a known bug (RGB-WG/rgb-node#208) that sometimes can cause the tests to hang.

We are still looking for a way to shutdown the rgb-node daemon process though. When that will be possible we could provide an rgb-lib method to shutdown.

About the blocking calls to the proxy, I don't think those are the issue here and moreover we need them to be blocking

@monaka
Copy link
Contributor Author

monaka commented Oct 28, 2022

I suppose this issue is not related to RGB-WG/rgb-node#208.
The root cause looks:

  • My app is built with Tokio asynchronous framework.
  • ConsignmentProxy uses reqwest::blocking module.

reqwest::blocking uses tokio::runtime.
https://github.com/seanmonstar/reqwest/blob/231b18f83572836c674404b33cb1ca8b35ca3e36/src/blocking/client.rs#L973
So my app and rgb-lib conflicts.

I'm inspecting how to fix the conflict with no API breakage.

@zoedberg
Copy link
Member

Sorry, I've misread the logs, you're right about conflicts between reqwest blocking and tokio async. Have you tried this solution seanmonstar/reqwest#1017 (comment) ?

@monaka
Copy link
Contributor Author

monaka commented Nov 7, 2022

@zoedberg Thank you for your suggestion.
But unfortunately, it looks that spawn_blocking doesn't help this issue.
I attach a part of the error message.

error: generator cannot be sent between threads safely
   --> src/wallet/mod.rs:125:35
    |
125 |         let resp = spawn_blocking(move || test::call_service(&app, req)).await;
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ generator is not `Send`
    |
    = help: within `impl std::future::Future<Output = <impl actix_web::dev::Service<actix_http::requests::request::Request, Response = ServiceResponse, Error = actix_web::Error> as actix_web::dev::Service<actix_http::requests::request::Request>>::Response>`, the trait `Send` is not implemented for `<impl actix_web::dev::Service<actix_http::requests::request::Request, Response = ServiceResponse, Error = actix_web::Error> as actix_web::dev::Service<actix_http::requests::request::Request>>::Future`

@monaka monaka mentioned this issue Nov 7, 2022
@zoedberg
Copy link
Member

zoedberg commented Nov 7, 2022

@monaka Please give a look at https://github.com/RGB-Tools/rgb-lib/tree/reqwest_async , I've switched to using the async version of reqwest and hence removed its blocking feature that should be causing your issue. If you can please confirm that this fixes your issue, I will then merge this in master.

@monaka
Copy link
Contributor Author

monaka commented Nov 14, 2022

@zoedberg Thank you for your kind support.
All my tests with https://github.com/RGB-Tools/rgb-lib/tree/reqwest_async are passed.
Please merge your patch to master.

@monaka monaka closed this as completed Nov 14, 2022
@nicbus nicbus changed the title How to avoid panic in shutdown.rs ? support tokio async runtimes Nov 14, 2022
@nicbus
Copy link
Member

nicbus commented Nov 14, 2022

merged in master

@monaka
Copy link
Contributor Author

monaka commented Nov 18, 2022

Let me re-open this.
I had the same issue in another code.

I attach a part of the log. (Guess it might be useless...).

thread 'main' panicked at 'Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.', /home/monaka/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:516:26

My failed test calls go_online().
I'm not sure but this case may be caused by dependent libs.

@monaka monaka reopened this Nov 18, 2022
@monaka
Copy link
Contributor Author

monaka commented Nov 18, 2022

Just guess but it may be caused by this line.

std::thread::sleep(Duration::from_millis(500));

@nicbus
Copy link
Member

nicbus commented Nov 21, 2022

could you please provide the code to reproduce the issue? if you can't share the code, a minimal example is fine as well

@monaka
Copy link
Contributor Author

monaka commented Nov 24, 2022

@monaka
Copy link
Contributor Author

monaka commented Nov 24, 2022

I found some thread::spawn in _go_online.

thread::spawn(move || {

thread::spawn(move || {

Just guess but I think they will conflict with Tokio.

@monaka
Copy link
Contributor Author

monaka commented Nov 27, 2022

How to reproduce:

rustup update
rm -fr /tmp/shiro-wallet
mkdir -p /tmp/shiro-wallet
git clone https://github.com/diamondhands-dev/shiro-backend.git -b pr-go_online-PUT-endpoint
cd shiro-backend/
docker ps -aq | xargs docker kill
docker system prune --force
test-mocks/start_services.sh
RUST_TEST_THREADS=1 BITCOIN_NETWORK_NAME=regtest cargo test
$ rustup -V
rustup 1.25.1 (bb60b1e89 2022-07-12)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.65.0 (897e37553 2022-11-02)`

@monaka
Copy link
Contributor Author

monaka commented Nov 27, 2022

I inspected this issue a little bit deeper.
I suppose it is not caused by std::thread::{sleep,spawn} but a nested block_on call.

  1. Actix-web calls tokio::runtime::scheduler::current_thread::CurrentThread::block_on().
  2. my async block calls rgb_lib::wallet::Wallet::go_online().
  3. The proxy code in rgb-lib calls block_on() same as above.
  4. The test failed by thread 'main' panicked at 'Cannot start a runtime from within a runtime.

@monaka
Copy link
Contributor Author

monaka commented Nov 28, 2022

Just a guess but this issue can be fixed by using tokio::task::spawn_blocking ?
I'm checking now.

@monaka
Copy link
Contributor Author

monaka commented Nov 28, 2022

Just a guess but this issue can be fixed by using tokio::task::spawn_blocking ?

Bingo.

It can be resolved by using spawn_blocking (in tokio::task or actix_web::rt::task).

@nicbus @zoedberg This issue can be resolved on my appication side.
I close this again. Thank you for your kind support.

@monaka monaka closed this as completed Nov 28, 2022
@zoedberg
Copy link
Member

@monaka I have a doubt that this issue could have been solved using spawn_blocking since the beginning. Could you please push your latest shiro-backend code (on the main branch tests still fail) so that I can do some experiments? I would be really happy if we could revert 5211dd8

@monaka
Copy link
Contributor Author

monaka commented Dec 2, 2022

@zoedberg Surely CI builds on shiro-backend (main branch) is failing.
But it is just caused by Codecov and all tests are passed now.
I agree it is reasonable to revert 5211dd8 even though some additional fixes are required to shiro-backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants