Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Upgrade tokio to 1.22.0 and replace async-std with tokio #12646

Merged
merged 22 commits into from
Dec 5, 2022

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Nov 8, 2022

Resolves #12499.

polkadot companion: paritytech/polkadot#6262

cumulus companion: paritytech/cumulus#1917

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Nov 8, 2022
@dmitry-markin dmitry-markin marked this pull request as draft November 8, 2022 11:16
@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Nov 8, 2022

Doesn't work yet, because the tests rely on async-std being used. Converted into draft.

Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

Overall LGTM but I have two questions:

  • is there a reason why async_std::spawn in tests is not converted to tokio::spawn?
  • the example code in rust-libp2p repository instructs to configure executor for the swarm in tokio chat example. Should we configure the default executor similarly in case params.executor is None in service.rs?

I'm paranoid about the second bullet because I burned two weeks debugging bizarre async execution issues after having configured libp2p incorrectly.

@koute
Copy link
Contributor

koute commented Nov 10, 2022

Nice! 👍

We should completely nuke async-std so that it isn't used anywhere in our codebases. Doesn't really make sense to have two different executors, especially when tokio's solid.

@dmitry-markin dmitry-markin added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Nov 10, 2022
@dmitry-markin
Copy link
Contributor Author

  • the example code in rust-libp2p repository instructs to configure executor for the swarm in tokio chat example. Should we configure the default executor similarly in case params.executor is None in service.rs?

I'm paranoid about the second bullet because I burned two weeks debugging bizarre async execution issues after having configured libp2p incorrectly.

Interestingly, the PR libp2p/rust-libp2p#3097 has just landed in libp2p that makes the specification of executor explicit during the construction of Swarm: SwarmBuilder::new has been replaced with SwarmBuilder::with_executor + some alternative constructors with predifined executors for tokio, async-std, etc.

I'm going to change the API of NetworkWorker::new so it won't accept None executor anymore in this PR, and later when libp2p-0.50 is released we can move to the new SwarmBuilder::with_executor constructor.

CC @altonen

@dmitry-markin dmitry-markin changed the title Upgrade tokio and replace async-std libp2p primitives with tokio ones Upgrade tokio to 1.22.0 and replace async-std with tokio Nov 27, 2022
@dmitry-markin
Copy link
Contributor Author

I suspect that I have converted some futures::executor::block_on into runtime.block_on where it wasn't strictly necessary. I also don't like that in some tests we create tokio runtime explicitly and split sync initialization and async execution, and in others we just run everything under #[tokio::test]. So, may be a follow-up PR/commits are needed to clean these things up @altonen @bkchr.

@dmitry-markin dmitry-markin marked this pull request as ready for review November 27, 2022 18:07
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 27, 2022
@melekes
Copy link
Contributor

melekes commented Nov 28, 2022

libp2p 0.50 was released last week. here is the PR that updates substrate: #12734

let me know if you want me to change executors to tokio.

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

use sc_block_builder::BlockBuilderProvider;
use sc_client_api::BlockBackend;

sp_tracing::try_init_simple();

let mut net = BeefyTestNet::new(2);
let runtime = Runtime::new().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

dummy question: can't we just use #[tokio::test]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. I think, historically some tests were synchronous and later converted to async ones with manual runtime instantiation, and I just repeated the mistake and did the same in other places. Probably this doesn't make sense, so I'm going to convert all these tests to #[tokio::test] in a follow-up PR.

client/network/src/config.rs Show resolved Hide resolved
@@ -87,7 +93,7 @@ fn notifications_state_consistent() {
);
}

async_std::task::block_on(async move {
runtime.block_on(async move {
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope we can get rid of all these block_on calls in the future in some follow-up PR

@dmitry-markin
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 2bde8c1 into master Dec 5, 2022
@paritytech-processbot paritytech-processbot bot deleted the dm-upgrade-tokio-and-libp2p-primitives branch December 5, 2022 08:18
@dmitry-markin dmitry-markin linked an issue Dec 7, 2022 that may be closed by this pull request
ltfschoen pushed a commit to ltfschoen/substrate that referenced this pull request Feb 22, 2023
…12646)

* Replace deprecated libp2p feature specs with correct ones

* Bump tokio to 1.21.2

* Replace async-std libp2p primitives with tokio ones

* minor: rustfmt

* Fix TestNet to run initialization in the tokio context

* Convert telemetry test from async-std to tokio

* Convert notifications tests from async-std to tokio

* Convert chain sync tests from async-std to tokio

* Ditch async-std completely

* Make executor mandatory

* Bump tokio to 1.22.0

* minor: rustfmt

* Explicitly use tokio runtime in tests

* Move more tests to explicit tokio runtime

* Explicitly set multithreaded runtime in tokio test

* minor: rustfmt

* minor: fix comment

* Replace async-std with tokio in MMR tests
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…12646)

* Replace deprecated libp2p feature specs with correct ones

* Bump tokio to 1.21.2

* Replace async-std libp2p primitives with tokio ones

* minor: rustfmt

* Fix TestNet to run initialization in the tokio context

* Convert telemetry test from async-std to tokio

* Convert notifications tests from async-std to tokio

* Convert chain sync tests from async-std to tokio

* Ditch async-std completely

* Make executor mandatory

* Bump tokio to 1.22.0

* minor: rustfmt

* Explicitly use tokio runtime in tests

* Move more tests to explicit tokio runtime

* Explicitly set multithreaded runtime in tokio test

* minor: rustfmt

* minor: fix comment

* Replace async-std with tokio in MMR tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade libp2p primitives to use tokio implementations Try switch sockets to tokio
6 participants