rpc v2: backpressure `chainhead_v1_follow` #6058

pkhry · 2024-10-14T17:28:50Z

Description

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 · 2024-10-14T17:39:59Z

bot fmt

command-bot · 2024-10-14T17:40:04Z

@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.

command-bot · 2024-10-14T17:43:36Z

@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.

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

niklasad1 · 2024-10-15T10:48:18Z

substrate/client/rpc/src/utils.rs

 	where
-		S: Stream<Item = T> + Unpin + Send + 'static,


ok nice catch :)

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

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

niklasad1 · 2024-10-15T13:41:44Z

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

@@ -53,6 +53,10 @@ use std::{
 /// `Initialized` event.
 const MAX_FINALIZED_BLOCKS: usize = 16;

+/// Maximum amount of events buffered by submit_events
+/// before dropping the stream.
+const MAX_BUFFERED_EVENTS: usize = 4;


Previously we used much bigger limit for this and @lexnv suggested to keep it similar to the warning limit because the number of chainHead subscriptions per connection is limited to four.

I would just make it 512 to start with, as it was before

/cc @lexnv what do you reckon?

512 sounds good to me

prdoc/pr_6058.prdoc

pkhry · 2024-10-15T14:32:35Z

substrate/client/rpc-spec-v2/src/chain_head/tests.rs

+		events.push(event);
+	}
+
+	assert_eq!(events.len(), 64);


dq: i'm not that familiar with testing libs but why subscription falls out at 63 instead of 511 here?
the error from overflowing buffer still happens tho
cc: @niklasad1

2024-10-15T15:07:45.783080Z TRACE jsonrpsee-server: [Methods::subscribe] Method: chainHead_v1_follow, params: Some(RawValue([false])) 2024-10-15T15:07:45.783324Z DEBUG rpc-spec-v2: [follow][id="8999421366389847"] Subscription accepted 2024-10-15T15:07:45.783524Z INFO sc_rpc::utils: Starting subscription=chainHead_v1_followEvent 2024-10-15T15:07:45.783642Z TRACE jsonrpsee-server: [Methods::inner_call] Method: chainHead_v1_follow, response: {"jsonrpc":"2.0","id":0,"result":8999421366389847} 2024-10-15T15:07:47.380636Z DEBUG jsonrpsee-server: [Subscription::next]: rx {"jsonrpc":"2.0","method":"chainHead_v1_followEvent","params":{"subscription":8999421366389847,"result":{"event":"initialized","finalizedBlockHashes":["0xb4b48648e4869cffd7333c79458c3db011b9d6556af2a53eb5a186763837a73c"]}}} 2024-10-15T15:07:47.382317Z DEBUG rpc-spec-v2: [follow][id="8999421366389847"] Failed to handle stream notification ExceededLimits 2024-10-15T15:07:47.382353Z DEBUG rpc-spec-v2: [follow][id="8999421366389847"] Failed to handle stream notification SubscriptionAbsent 2024-10-15T15:07:47.383164Z DEBUG rpc-spec-v2: [follow][id="8999421366389847"] Subscription removed

It looks like you are hitting max block pinning limits when you import so many blocks.
Thus, it may be better to just add subscription buffer cap to the ChainHeadConfig to make it easy to change for tests etc and it's a bit extreme to import 512 blocks for a unit test.

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

niklasad1 · 2024-10-15T15:22:52Z

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

-					let _ = sink.send(&FollowEvent::<String>::Stop).await;
-					// No need to propagate this error further, the client disconnected.
-					return Ok(())
+		let stream = stream


It's a bit tempting to add something similar to pipe_from_stream that takes a TryStream i.,e that produces Result<T, Error> and bails on the first error similar to how try_for_each works to avoid having this channel to be injected here just to get back the error and then quit....

Nothing that needs to be addressed in this PR but would be neat :)

added to the pr

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

lexnv

LGTM! Thanks for handling this @pkhry 🙏

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs

prdoc/pr_6058.prdoc

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>

add backpressure for chainhead_follow_v1

685016a

pkhry added the T3-RPC_API This PR/Issue is related to RPC APIs. label Oct 14, 2024

pkhry changed the title ~~rpc v2: backpressure chainhead_follow_v1~~ rpc v2: backpressure chainhead_v1_follow Oct 14, 2024

pkhry and others added 2 commits October 14, 2024 19:39

prdoc

3a836e1

Merge branch 'master' into pkhry/backressure_chainhead_follow

07d22ef

pkhry requested a review from niklasad1 October 14, 2024 17:40

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

review comments

cdd8977

pkhry requested a review from niklasad1 October 15, 2024 13:22

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

prdoc/pr_6058.prdoc Show resolved Hide resolved

review comments

1fe04cd

pkhry commented Oct 15, 2024

View reviewed changes

lexnv self-requested a review October 15, 2024 15:00

lexnv reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

lexnv reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 approved these changes Oct 15, 2024

View reviewed changes

lexnv reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

lexnv approved these changes Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 15, 2024

View reviewed changes

substrate/client/rpc-spec-v2/src/chain_head/chain_head_follow.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 16, 2024

View reviewed changes

prdoc/pr_6058.prdoc Outdated Show resolved Hide resolved

pkhry and others added 6 commits October 16, 2024 11:00

Apply suggestions from code review

092949c

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>

review comments

40a7fa6

review

58361ab

Merge branch 'master' into pkhry/backressure_chainhead_follow

60fffcb

review comments

6fb3986

Merge branch 'master' into pkhry/backressure_chainhead_follow

ced44aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc v2: backpressure `chainhead_v1_follow` #6058

rpc v2: backpressure `chainhead_v1_follow` #6058

pkhry commented Oct 14, 2024 •

edited

Loading

pkhry commented Oct 14, 2024

command-bot bot commented Oct 14, 2024 •

edited

Loading

command-bot bot commented Oct 14, 2024

niklasad1 Oct 15, 2024

lexnv Oct 15, 2024

niklasad1 Oct 15, 2024 •

edited

Loading

pkhry Oct 15, 2024

lexnv Oct 15, 2024

pkhry Oct 15, 2024

niklasad1 Oct 15, 2024 •

edited

Loading

niklasad1 Oct 15, 2024 •

edited

Loading

pkhry Oct 17, 2024

lexnv left a comment

rpc v2: backpressure chainhead_v1_follow #6058

Are you sure you want to change the base?

rpc v2: backpressure chainhead_v1_follow #6058

Conversation

pkhry commented Oct 14, 2024 • edited Loading

Description

Integration

Review Notes

pkhry commented Oct 14, 2024

command-bot bot commented Oct 14, 2024 • edited Loading

command-bot bot commented Oct 14, 2024

niklasad1 Oct 15, 2024

Choose a reason for hiding this comment

lexnv Oct 15, 2024

Choose a reason for hiding this comment

niklasad1 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

pkhry Oct 15, 2024

Choose a reason for hiding this comment

lexnv Oct 15, 2024

Choose a reason for hiding this comment

pkhry Oct 15, 2024

Choose a reason for hiding this comment

niklasad1 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

niklasad1 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

pkhry Oct 17, 2024

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

rpc v2: backpressure `chainhead_v1_follow` #6058

rpc v2: backpressure `chainhead_v1_follow` #6058

pkhry commented Oct 14, 2024 •

edited

Loading

command-bot bot commented Oct 14, 2024 •

edited

Loading

niklasad1 Oct 15, 2024 •

edited

Loading

niklasad1 Oct 15, 2024 •

edited

Loading

niklasad1 Oct 15, 2024 •

edited

Loading