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

refactor: Upgraded all the dependencies #3869

Merged
merged 10 commits into from
Feb 3, 2021

Conversation

frol
Copy link
Collaborator

@frol frol commented Jan 29, 2021

The main goal is to get the latest actix/actix-web along with tokio 1.x. Fixes #3868

Extra bonuses:

  1. Resolved compilation errors due to use of private modules of syn and serde crates in our code-base
  2. Hopefully it will address a long-standing Telemetry issue "not an error"
  3. It may also address a memory leak in Telemetry

@frol
Copy link
Collaborator Author

frol commented Jan 29, 2021

Copy link
Contributor

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

Amazing! 🥇

Of note: fixes #3868

tokio = { version = "0.2", features = ["full"] }
actix = "0.11.0-beta.1"
actix-web = "4.0.0-beta.1"
actix-cors = { git = "https://github.com/andy128k/actix-extras.git", branch="update-dependencies" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on a random branch seems brittle to me. What happens if the user deletes the branch? Further down you list the dependency as 0.5.4; is there something in particular we need here that is not needed there? If so, maybe we should make our own fork of actix-extras where we can keep this branch around until we are able to upgrade to the next release of actix-cors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need beta versions of actix with tokio 1.x support, and actix-cors has not been released with the version bump yet. Forking to our guthub org makes perfect sense, I will do that 👍

@frol
Copy link
Collaborator Author

frol commented Jan 29, 2021

Extra bonuses:

  1. Resolved compilation errors due to use of private modules of syn and serde crates in our code-base
  2. Hopefully it will address a long-standing Telemetry issue "not an error"
  3. It may also address a memory leak in Telemetry

@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented Jan 29, 2021

@frol looks like the PR doesn't yet compile. Are you going to address it any time soon? We need it asap to address #3854

EDIT: looks like neard compiles fine, but it is still desirable to get this PR to a ready state sooner than later

@frol
Copy link
Collaborator Author

frol commented Jan 29, 2021

It passed initial checks locally, but I expected it to fail somewhere anyway. I left my home office for a walk, and I will be back in 4-5 hours. If you have a bandwidth to work on it before I get back, go for it, otherwise, I was going to address CI issues during the weekend to minimize interruption and have a chance to run full nayduck test

@bowenwang1996
Copy link
Collaborator

I investigated the CI failure and realized that it is not very easy to fix:

I am not sure how to best proceed at this point. I feel like we can either pin some syn dependency or try having two different ethereum-types version in the same crate. @frol @artob any suggestions?

@bowenwang1996
Copy link
Collaborator

@frol also looks like there is some change to maybe actix runtime that causes trouble when we start a node:

thread 'main' panicked at '`spawn_local` called from outside of a `task::LocalSet`', /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.1.0/src/ta\
sk/local.rs:303:18
stack backtrace:
   0: rust_begin_unwind
             at /rustc/91a79fb29ac78d057d04dbe86be13d5dcc64309a/library/std/src/panicking.rs:483
   1: core::panicking::panic_fmt
             at /rustc/91a79fb29ac78d057d04dbe86be13d5dcc64309a/library/core/src/panicking.rs:85
   2: core::option::expect_failed
             at /rustc/91a79fb29ac78d057d04dbe86be13d5dcc64309a/library/core/src/option.rs:1226
   3: core::option::Option<T>::expect
             at /Users/bowenwang/.rustup/toolchains/nightly-2020-10-08-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:346
   4: tokio::task::local::spawn_local::{{closure}}
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.1.0/src/task/local.rs:302
   5: tokio::macros::scoped_tls::ScopedKey<T>::with
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.1.0/src/macros/scoped_tls.rs:72
   6: tokio::task::local::spawn_local
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.1.0/src/task/local.rs:301
   7: actix_rt::arbiter::Arbiter::spawn
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-rt-2.0.0-beta.2/src/arbiter.rs:162
   8: actix_rt::spawn
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-rt-2.0.0-beta.2/src/lib.rs:35
   9: actix_http::client::pool::ConnectionPool<T,Io>::new
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-http-3.0.0-beta.1/src/client/pool.rs:67
  10: actix_http::client::connector::Connector<T,U>::finish
             at /Users/bowenwang/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-http-3.0.0-beta.1/src/client/connector.rs:344
  11: near_telemetry::TelemetryActor::new
             at /Users/bowenwang/NEAR/nearcore/chain/telemetry/src/lib.rs:50
  12: neard::start_with_config
             at /Users/bowenwang/NEAR/nearcore/neard/src/lib.rs:227
  13: neard::main
             at /Users/bowenwang/NEAR/nearcore/neard/src/main.rs:236
  14: core::ops::function::FnOnce::call_once
             at /Users/bowenwang/.rustup/toolchains/nightly-2020-10-08-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:227
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Panic in Arbiter thread.

@birchmd
Copy link
Contributor

birchmd commented Jan 29, 2021

I made some changes to main.rs which seem to fix the panicked at spawn_local issue. I'm not 100% sure yet if it is correct, but the node seems to run ok with the change.

diff --git a/neard/src/lib.rs b/neard/src/lib.rs
index 92c3db80..18deb412 100644
--- a/neard/src/lib.rs
+++ b/neard/src/lib.rs
@@ -214,7 +214,6 @@ pub fn start_with_config(
     config: NearConfig,
 ) -> (Addr<ClientActor>, Addr<ViewClientActor>, Vec<Arbiter>) {
     let store = init_and_migrate_store(home_dir, &config);
-    near_actix_utils::init_stop_on_panic();

     let runtime = Arc::new(NightshadeRuntime::new(
         home_dir,
diff --git a/neard/src/main.rs b/neard/src/main.rs
index 814dfeb5..41ae7ce9 100644
--- a/neard/src/main.rs
+++ b/neard/src/main.rs
@@ -232,10 +232,11 @@ fn main() {
                 near_config.client_config.archive = true;
             }

-            let system = System::new("NEAR");
-            let (_, _, arbiters) = start_with_config(home_dir, near_config);
-            system.run().unwrap();
-            arbiters.into_iter().for_each(|mut a| a.join().unwrap());
+            System::builder().name("NEAR").stop_on_panic(true).run(move || {
+                start_with_config(home_dir, near_config);
+            }).unwrap();
         }
         ("unsafe_reset_data", Some(_args)) => {
             let store_path = get_store_path(home_dir);

@frol
Copy link
Collaborator Author

frol commented Feb 1, 2021

@bowenwang1996 I forked openethereum and upgraded the dependencies. I had also forked actix-extras and paperclip (we use it for RosettaRPC). Now, we have a single version of actix (0.11.0-beta.1), actix-web (4.0.0-beta.1), and tokio (1.1.1)!

@birchmd Thanks for the patch! I haven't yet confirmed if that fixes the issue, but it looks reasonable to me.

@frol frol marked this pull request as ready for review February 3, 2021 19:55
@frol
Copy link
Collaborator Author

frol commented Feb 3, 2021

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.

Upgrade actix and tokio dependencies
5 participants