This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Use tokio runtime handle instead of TaskExecutor abstraction#9737
Merged
7 commits merged intomasterfrom Sep 12, 2021
Merged
Conversation
bkchr
added
A0-please_review
Before this pr we had the `TaskExecutor` abstraction which theoretically allowed that any futures executor could have been used. However, this was never tested and is currently not really required. Anyone running a node currently only used tokio and nothing else (because this was hard coded in CLI). So, this pr removes the `TaskExecutor` abstraction and relies directly on the tokio runtime handle. Besides this changes, this pr also makes sure that the http and ws rpc server use the same tokio runtime. This fixes a panic that occurred when you drop the rpc servers inside an async function (tokio doesn't like that a tokio runtime is dropped in the async context of another tokio runtime). As we don't use any custom runtime in the http rpc server anymore, this pr also removes the `rpc-http-threads` cli argument. If external parties complain that there aren't enough threads for the rpc server, we could bring support for increasing the thread count of the tokio runtime.
bkchr
force-pushed
the
bkchr-forward-tokio
branch
from
f774459 to
01b5a3f
Compare
bkchr
commented
Sep 9, 2021
client/rpc-servers/src/lib.rs
Outdated
Comment on lines
+147
to
+148
| .threads(1) | ||
| .event_loop_executor(tokio_handle) |
Member
Author
There was a problem hiding this comment.
I read the docs and checked the code.
I'm almost sure, but can someone confirm that this is correct?
Contributor
There was a problem hiding this comment.
/cc @tomusdrw but given https://docs.rs/jsonrpc-http-server/18.0.0/jsonrpc_http_server/struct.ServerBuilder.html#method.threads sounds correct to me
Member
Author
There was a problem hiding this comment.
By correct I mean that we are still answering requests in parallel.
Contributor
There was a problem hiding this comment.
yeah, I was referring to
So it’s also possible to run a multi-threaded server by passing the default tokio::runtime executor to this builder and setting threads to 1.
Member
Author
There was a problem hiding this comment.
Yeah, my second comment was just to make my first one more clear xD
ordian
approved these changes
Sep 9, 2021
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
I don't know how to review this properly. Ideally, this PR could use a burnin on a RPC node, but I'm not sure it's warranted.
I understand that we lose the TaskExecutor abstraction here, that might worth adding back later, but that's probably out of scope here .
Other than that, looks good to me.
Member
Author
This custom http thread number wasn't used by us, I will ping the people that requested it.
It was currently not used anyway and I think it may also not worked with other executors, because hyper for example requires Tokio. However, as we will refactor a lot of things, this is one of the first things that would have been gone anyway.
👍 |
niklasad1
approved these changes
Sep 10, 2021
Contributor
niklasad1
left a comment
niklasad1
left a comment
There was a problem hiding this comment.
LGTM, but yeah would good check with users of the --rpc http threads
Member
Author
|
bot merge |
|
Trying merge. |
ghost
merged commit
shawntabrizi
added a commit
to paritytech/cumulus
that referenced
this pull request
Sep 15, 2021
shawntabrizi
added a commit
to paritytech/cumulus
that referenced
this pull request
Sep 15, 2021
bkchr
added a commit
to paritytech/cumulus
that referenced
this pull request
Sep 16, 2021
* Remove event pallet::metadata attributes * Add scale-info deps, TypeInfo derives, update call variants * Update metadata runtime APIs * Add missing scale_info dependency, update rococo runtime API * Add missing scale_info dependency * Remove pushed diener patches * Cargo.lock * Add missing scale-info dependencies * Fixes * Statemint runtime fixes * Call struct variant empty matches * Add missing scale-info dependency * Fixes * scale-info 1.0 * cargo update -p xcm * update lock * Update Cargo.lock * update to latest polkadot * remove rpc_http_threads paritytech/substrate#9737 * replace task executor with tokio handler paritytech/substrate#9737 * fix test compilation? * Update Cargo.lock * cargo update * remove unused * Update substrate and polkadot * Update test/client/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
WebWizrd8
added a commit
to WebWizrd8/cumulus
that referenced
this pull request
Nov 21, 2022
* Remove event pallet::metadata attributes * Add scale-info deps, TypeInfo derives, update call variants * Update metadata runtime APIs * Add missing scale_info dependency, update rococo runtime API * Add missing scale_info dependency * Remove pushed diener patches * Cargo.lock * Add missing scale-info dependencies * Fixes * Statemint runtime fixes * Call struct variant empty matches * Add missing scale-info dependency * Fixes * scale-info 1.0 * cargo update -p xcm * update lock * Update Cargo.lock * update to latest polkadot * remove rpc_http_threads paritytech/substrate#9737 * replace task executor with tokio handler paritytech/substrate#9737 * fix test compilation? * Update Cargo.lock * cargo update * remove unused * Update substrate and polkadot * Update test/client/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
ggwpez
pushed a commit
to ggwpez/runtimes
that referenced
this pull request
Mar 10, 2023
* Remove event pallet::metadata attributes * Add scale-info deps, TypeInfo derives, update call variants * Update metadata runtime APIs * Add missing scale_info dependency, update rococo runtime API * Add missing scale_info dependency * Remove pushed diener patches * Cargo.lock * Add missing scale-info dependencies * Fixes * Statemint runtime fixes * Call struct variant empty matches * Add missing scale-info dependency * Fixes * scale-info 1.0 * cargo update -p xcm * update lock * Update Cargo.lock * update to latest polkadot * remove rpc_http_threads paritytech/substrate#9737 * replace task executor with tokio handler paritytech/substrate#9737 * fix test compilation? * Update Cargo.lock * cargo update * remove unused * Update substrate and polkadot * Update test/client/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
ggwpez
pushed a commit
to ggwpez/runtimes
that referenced
this pull request
Mar 14, 2023
* Remove event pallet::metadata attributes * Add scale-info deps, TypeInfo derives, update call variants * Update metadata runtime APIs * Add missing scale_info dependency, update rococo runtime API * Add missing scale_info dependency * Remove pushed diener patches * Cargo.lock * Add missing scale-info dependencies * Fixes * Statemint runtime fixes * Call struct variant empty matches * Add missing scale-info dependency * Fixes * scale-info 1.0 * cargo update -p xcm * update lock * Update Cargo.lock * update to latest polkadot * remove rpc_http_threads paritytech/substrate#9737 * replace task executor with tokio handler paritytech/substrate#9737 * fix test compilation? * Update Cargo.lock * cargo update * remove unused * Update substrate and polkadot * Update test/client/src/lib.rs Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this pr we had the
TaskExecutorabstraction which theoreticallyallowed that any futures executor could have been used. However, this
was never tested and is currently not really required. Anyone running a
node currently only used tokio and nothing else (because this was hard
coded in CLI). So, this pr removes the
TaskExecutorabstraction andrelies directly on the tokio runtime handle.
Besides this changes, this pr also makes sure that the http and ws rpc
server use the same tokio runtime. This fixes a panic that occurred when
you drop the rpc servers inside an async function (tokio doesn't like
that a tokio runtime is dropped in the async context of another tokio
runtime).
As we don't use any custom runtime in the http rpc server anymore, this
pr also removes the
rpc-http-threadscli argument. If external partiescomplain that there aren't enough threads for the rpc server, we could
bring support for increasing the thread count of the tokio runtime.
polkadot companion: paritytech/polkadot#3830