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

Shutdown runtime after task is completed #331

Merged
merged 3 commits into from
Feb 14, 2020

Conversation

jaspervdm
Copy link
Contributor

@jaspervdm jaspervdm commented Feb 12, 2020

Each request was spawning its own multithreaded runtime that kept running even after the request was done, taking up resources unnecessarily. This PR addresses that, by using a runtime that runs in the current thread.
Ideally we would have a single multithreaded runtime to spawn all requests on, but that is a larger refactor that I'm planning on doing later on.

Small side note is that the current-thread runtime is only active until the future completes and any unfinished tasks that were spawned by this future aren't polled anymore. Since we are not spawning any tasks that outlive our future this shouldn't have any effect but it is something to keep in the back of our minds.

Fixes #330

@jaspervdm jaspervdm requested a review from yeastplume February 12, 2020 23:01
@fentanyluser
Copy link

with this i am receiving these errors

20200213 00:14:35.309 ERROR grin_util::logger -
thread 'tokio-runtime-worker-0' panicked at 'Multiple executors at once: EnterError { reason: "attempted to run an executor while another executor is already running" }': src/libcore/result.rs:1165stack backtrace:
0: grin_util::logger::send_panic_to_log::{{closure}}
1: std::panicking::rust_panic_with_hook
at src/libstd/panicking.rs:468
2: std::panicking::continue_panic_fmt
at src/libstd/panicking.rs:373
3: rust_begin_unwind
at src/libstd/panicking.rs:302
4: core::panicking::panic_fmt
at src/libcore/panicking.rs:139
5: core::result::unwrap_failed
at src/libcore/result.rs:1165
6: tokio::runtime::current_thread::runtime::Runtime::block_on
7: grin_wallet_impls::client_utils::client::Client::send_request
8: grin_wallet_impls::client_utils::client::Client::get
9: <grin_wallet_impls::node_clients::http::HTTPNodeClient as grin_wallet_libwallet::types::NodeClient>::get_chain_tip
10: grin_wallet_libwallet::api_impl::owner::node_height
11: grin_wallet_api::owner::Owner<L,C,K>::node_height
12: <grin_wallet_api::owner::Owner<L,C,K> as grin_wallet_api::owner_rpc::OwnerRpc>::node_height
13: ::handle
14: easy_jsonrpc_mw::handle_call
15: easy_jsonrpc_mw::handle_parsed_request
16: easy_jsonrpc_mw::Handler::handle_request
17: futures::future::chain::Chain<A,B,C>::poll
18: <futures::future::and_then::AndThen<A,B,F> as futures::future::Future>::poll
19: futures::future::chain::Chain<A,B,C>::poll
20: <futures::future::and_then::AndThen<A,B,F> as futures::future::Future>::poll
21: futures::future::chain::Chain<A,B,C>::poll
22: <futures::future::or_else::OrElse<A,B,F> as futures::future::Future>::poll
23: <hyper::proto::h1::dispatch::Server as hyper::proto::h1::dispatch::Dispatch>::poll_msg
24: <futures::future::either::Either<A,B> as futures::future::Future>::poll
25: <hyper::server::conn::upgrades::UpgradeableConnection<I,S,E> as futures::future::Future>::poll
26: <hyper::server::conn::spawn_all::NewSvcTask<I,N,S,E,W> as futures::future::Future>::poll
27: futures::task_impl::std::set
28: std::panicking::try::do_call
29: __rust_maybe_catch_panic
at src/libpanic_unwind/lib.rs:78
30: tokio_threadpool::task::Task::run
31: tokio_threadpool::worker::Worker::run_task
32: tokio_threadpool::worker::Worker::run
33: tokio_reactor::with_default
34: tokio::runtime::builder::Builder::build::{{closure}}
35: std::thread::local::LocalKey::with
36: std::thread::local::LocalKey::with
37: std::sys_common::backtrace::__rust_begin_short_backtrace
38: std::panicking::try::do_call
39: __rust_maybe_catch_panic
at src/libpanic_unwind/lib.rs:78
40: core::ops::function::FnOnce::call_once{{vtable.shim}}
41: <alloc::boxed::Box as core::ops::function::FnOnce>::call_once
at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
42: <alloc::boxed::Box as core::ops::function::FnOnce>::call_once
at /rustc/73528e339aae0f17a15ffa49a8ac608f50c6cf14/src/liballoc/boxed.rs:942
std::sys_common::thread::start_thread
at src/libstd/sys_common/thread.rs:13
std::sys::unix::thread::Thread::new::thread_start
at src/libstd/sys/unix/thread.rs:79
43: start_thread
44: clone

Thread 'tokio-runtime-worker-0' panicked with message:
"Multiple executors at once: EnterError { reason: "attempted to run an executor while another executor is already running" }"
See .grin/main/grin-wallet.log for further details.

@jaspervdm
Copy link
Contributor Author

Hmm, I'll investigate tomorrow

@jaspervdm jaspervdm changed the title Use current thread for tokio runtime Shutdown runtime after task is completed Feb 13, 2020
@jaspervdm
Copy link
Contributor Author

@fentanyluser Should be fixed now, I tested the owner api and it no longer panics with latest commit in this PR. The original issue should also be fixed since we wait for shutdown of runtime before returning.

@yeastplume yeastplume added this to the v3.1.0 milestone Feb 14, 2020
Copy link
Member

@yeastplume yeastplume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, test failures unrelated so merging.

@yeastplume yeastplume merged commit 4774704 into mimblewimble:master Feb 14, 2020
@aleqx
Copy link

aleqx commented Feb 15, 2020

Getting a error[E0658]: non exhaustive is an experimental feature when trying to compile master with this commit. Does it need a rustup to nightly?

EDIT: Ignore me - I thought I was running the latest stable rustc but in fact it was quite old. My apologies

@quentinlesceller
Copy link
Member

Which version of rust are you using? @aleqx I'm not getting this on 1.41.

@fentanyluser
Copy link

It appears to be still having the same issue #330

@jaspervdm jaspervdm deleted the runtime branch February 17, 2020 11:11
antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
* Use current thread for tokio runtime

* Use single thread runtime with shutdown
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

Successfully merging this pull request may close these issues.

grin-wallet owner_api creating large quantity of threads
5 participants