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

rpc v2: backpressure chainhead_v1_follow #6058

Merged
merged 15 commits into from
Oct 18, 2024

Conversation

pkhry
Copy link
Contributor

@pkhry pkhry commented Oct 14, 2024

Description

closes #5871

The chainHead_v1_follow is using unbounded channels to send out messages on the JSON-RPC connection which may use lots of memory if the client is slow and can't really keep up with server i.e, substrate may keep lots of message in memory

This PR changes the outgoing stream to abort and send a Stop event downstream in the case that client doesn't keep up with the producer.

Integration

In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the R0-Silent label. In case of a R0-Silent, it can be ignored.

Review Notes

  • rpc::Subscription::pipe_from_stream - now takes Self param by reference, change was made to allow sending events to the Subscription after calls to pipe_from_stream.
  • chainhead_follow::submit_events - now uses Abortable stream to end it early in case
    • connection was closed by the client
    • signal received that subscription should stop
    • error has occured when processing the events
    • client can't keep up with the events produced
  • TODO:
    • make the abort logic less hacky

@pkhry pkhry added the T3-RPC_API This PR/Issue is related to RPC APIs. label Oct 14, 2024
@pkhry pkhry changed the title rpc v2: backpressure chainhead_follow_v1 rpc v2: backpressure chainhead_v1_follow Oct 14, 2024
@pkhry
Copy link
Contributor Author

pkhry commented Oct 14, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 14, 2024

@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7571594 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-068fee57-0c9c-42ce-9555-9427935c80cd to cancel this command or bot cancel to cancel all commands in this pull request.

@pkhry pkhry requested a review from niklasad1 October 14, 2024 17:40
@command-bot
Copy link

command-bot bot commented Oct 14, 2024

@pkhry Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7571594 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7571594/artifacts/download.

where
S: Stream<Item = T> + Unpin + Send + 'static,
Copy link
Member

Choose a reason for hiding this comment

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

ok nice catch :)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this was causing the lifetime issue Pavlo mentioned before, nice catch indeed :D

@pkhry pkhry requested a review from niklasad1 October 15, 2024 13:22
@lexnv lexnv self-requested a review October 15, 2024 15:00
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for handling this @pkhry 🙏

prdoc/pr_6058.prdoc Outdated Show resolved Hide resolved
@pkhry pkhry requested a review from niklasad1 October 18, 2024 08:30
@pkhry
Copy link
Contributor Author

pkhry commented Oct 18, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 18, 2024

@pkhry https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7598335 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 21-70da5a54-87f4-4db3-966c-444529c72eaf to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 18, 2024

@pkhry Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7598335 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7598335/artifacts/download.

@carlosala
Copy link

I guess this PR could be backported to 2407 and 2409 as well, similar to #5741 👍🏻

cc @niklasad1

@niklasad1 niklasad1 added the A4-needs-backport Pull request must be backported to all maintained releases. label Oct 18, 2024
@pkhry pkhry added this pull request to the merge queue Oct 18, 2024
Merged via the queue into master with commit a0aefc6 Oct 18, 2024
234 of 243 checks passed
@pkhry pkhry deleted the pkhry/backressure_chainhead_follow branch October 18, 2024 13:02
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6058-to-stable2407
git worktree add --checkout .worktree/backport-6058-to-stable2407 backport-6058-to-stable2407
cd .worktree/backport-6058-to-stable2407
git reset --hard HEAD^
git cherry-pick -x a0aefc6b233ace0a82a8631d67b6854e6aeb014b
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-6058-to-stable2409
git worktree add --checkout .worktree/backport-6058-to-stable2409 backport-6058-to-stable2409
cd .worktree/backport-6058-to-stable2409
git reset --hard HEAD^
git cherry-pick -x a0aefc6b233ace0a82a8631d67b6854e6aeb014b
git push --force-with-lease

pkhry added a commit that referenced this pull request Oct 21, 2024
closes #5871

> The chainHead_v1_follow is using unbounded channels to send out
messages on the JSON-RPC connection which may use lots of memory if the
client is slow and can't really keep up with server i.e, substrate may
keep lots of message in memory

This PR changes the outgoing stream to abort and send a `Stop` event
downstream in the case that client doesn't keep up with the producer.

*In depth notes about how this PR should be integrated by downstream
projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the `R0-Silent` label. In
case of a `R0-Silent`, it can be ignored.*

- `rpc::Subscription::pipe_from_stream` - now takes `Self` param by
reference, change was made to allow sending events to the `Subscription`
after calls to `pipe_from_stream`.
- `chainhead_follow::submit_events` - now uses `Abortable` stream to end
it early in case
     - connection was closed by the client
     - signal received that subscription should stop
     - error has occured when processing the events
     - client can't keep up with the events produced
- TODO:
  - make the abort logic less hacky

---------

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
(cherry picked from commit a0aefc6)
Signed-off-by: Pavlo Khrystenko <p.khrystenko@gmail.com>
@niklasad1 niklasad1 removed the A4-needs-backport Pull request must be backported to all maintained releases. label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RPC-Spec-V2] chainHead_v1_follow should be "bounded"
4 participants