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

rpc: backpressured RPC server (bump jsonrpsee 0.20) #13992

Open
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Apr 24, 2023

This is rather big change but most stuff are "chore", the major things in this bump are:

  • Server backpressure (the subscription impls are modified to deal with that)
  • Allow custom error types / return types (remove jsonrpsee::core::Error and jsonrpee::core::CallError)
  • Easier subscription API (spawned internally by jsonrpsee doesn't work well with rpc subscription prometheus, so these are spawned with oneshot to keep it alive)
  • Bug fixes (graceful shutdown in particular not used by substrate anyway)
  • Less dependencies for the clients in particular
  • Return type requires Clone in method call responses
  • tokio channels are now used and tests must run on the tokio runtime to not panic

Hopefully the last release prior to 1.0, sorry in advance for a big PR

polkadot companion: paritytech/polkadot#7211

cumulus companion: paritytech/cumulus#2560

@niklasad1 niklasad1 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 24, 2023
@niklasad1 niklasad1 added 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 Apr 25, 2023
@niklasad1 niklasad1 mentioned this pull request Apr 27, 2023
altaua pushed a commit that referenced this pull request Jun 13, 2023
This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as #13992 that also temporarily add a patch
section.
altaua pushed a commit that referenced this pull request Jun 13, 2023
This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as #13992 that also temporarily add a patch
section.
altaua pushed a commit that referenced this pull request Jun 14, 2023
This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as #13992 that also temporarily add a patch
section.
altaua pushed a commit that referenced this pull request Jun 14, 2023
This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as #13992 that also temporarily add a patch
section.
altaua pushed a commit that referenced this pull request Jun 14, 2023
This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as #13992 that also temporarily add a patch
section.
@niklasad1 niklasad1 requested a review from a team June 24, 2023 14:45
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…ch#14372)

This is less brittle than locally editing Cargo.toml, which breaks when
our devs submit PRs such as paritytech#13992 that also temporarily add a patch
section.
@stale
Copy link

stale bot commented Jul 30, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 30, 2023
@stale stale bot removed the A3-stale label Jul 31, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3295104

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3295105

@niklasad1 niklasad1 changed the title rpc: backpressured RPC server (bump jsonrpsee 0.18.2) rpc: backpressured RPC server (bump jsonrpsee 0.20) Aug 17, 2023
@kayabaNerve
Copy link

Sorry for walking in without context, yet what were the regressions identified preventing this from being merged?

@niklasad1
Copy link
Member Author

niklasad1 commented Aug 23, 2023

Sorry for walking in without context, yet what were the regressions identified preventing this from being merged?

A leak and that the new async subscription API allocates more memory.

The leak is now fixed but investigating a fix for the increased memory usage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the 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 T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.