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

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

Open
niklasad1 wants to merge 64 commits intomasterfrom
na-jsonrpsee-v0.18
Open

rpc: backpressured RPC server (bump jsonrpsee 0.20)#13992
niklasad1 wants to merge 64 commits intomasterfrom
na-jsonrpsee-v0.18

Conversation

@niklasad1
Copy link
Copy Markdown
Contributor

@niklasad1 niklasad1 commented Apr 24, 2023 •
edited
Loading

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 commented Apr 24, 2023
View reviewed changes
Comment thread client/rpc-servers/src/lib.rs Outdated
niklasad1
niklasad1 commented Apr 24, 2023
View reviewed changes
Comment thread client/rpc-servers/src/lib.rs Outdated
@serban300 serban300 mentioned this pull request Apr 25, 2023
Closed
niklasad1
niklasad1 commented Apr 25, 2023
View reviewed changes
Comment thread client/rpc/src/chain/chain_full.rs Outdated
@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
Closed
altaua pushed a commit that referenced this pull request Jun 13, 2023
cargo-hfuzz ci: specify manifest patches via cargo --config
ceeaef1 
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
cargo-hfuzz ci: specify manifest patches via cargo --config
41fb455 
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
cargo-hfuzz ci: specify manifest patches via cargo --config
fb6e3e0 
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
cargo-hfuzz ci: specify manifest patches via cargo --config
bd85fc0 
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
cargo-hfuzz ci: specify manifest patches via cargo --config (#14372)
85960c2 
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
@niklasad1 niklasad1 requested review from athei and koute as code owners June 24, 2023 14:45
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@nathanwhit
cargo-hfuzz ci: specify manifest patches via cargo --config (parityte…
80ba1dd 
…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
Copy Markdown

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
Copy Markdown

paritytech-cicd-pr commented Jul 31, 2023

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
Copy Markdown

paritytech-cicd-pr commented Jul 31, 2023

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
@niklasad1 niklasad1 mentioned this pull request Aug 23, 2023
Closed
2 tasks
@kayabaNerve
Copy link
Copy Markdown

kayabaNerve commented Aug 23, 2023

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

@niklasad1
Copy link
Copy Markdown
Contributor Author

niklasad1 commented Aug 23, 2023 •
edited
Loading

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.

@niklasad1 niklasad1 mentioned this pull request Oct 30, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andresilva andresilva Awaiting requested review from andresilva

@athei athei Awaiting requested review from athei athei is a code owner

@koute koute Awaiting requested review from koute koute is a code owner

4 more reviewers

@sam0x17 sam0x17 sam0x17 approved these changes

@skunert skunert skunert approved these changes

@liamaharon liamaharon liamaharon approved these changes

@lexnv lexnv lexnv approved these changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.