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

rpc servers CLI: add --max--subscriptions--per--connection + fix a few bugs #11461

Merged
paritytech-processbot merged 5 commits into from
May 21, 2022
Merged

rpc servers CLI: add --max--subscriptions--per--connection + fix a few bugs #11461

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8392760
cli: fix RPC CLI nits
niklasad1 May 18, 2022
e97a8a6
Merge remote-tracking branch 'origin/master' into na-fix-rpc-subscrip…
niklasad1 May 20, 2022
f39e142
remove needless lines
niklasad1 May 20, 2022
cb7ecda
cargo fmt
niklasad1 May 21, 2022
dfb6be8
Update client/service/src/lib.rs
niklasad1 May 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions client/cli/src/commands/run_cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@ pub struct RunCmd {
#[clap(long)]
pub rpc_max_response_size: Option<usize>,

/// Set the the maximum concurrent subscriptions per connection.
/// Default is 1024.
#[clap(long)]
pub rpc_max_subscriptions_per_connection: Option<usize>,

/// Expose Prometheus exporter on all interfaces.
///
/// Default is local.
Expand Down Expand Up @@ -459,6 +464,18 @@ impl CliConfiguration for RunCmd {
Ok(self.rpc_max_payload)
}

fn rpc_max_request_size(&self) -> Result<Option<usize>> {
Ok(self.rpc_max_request_size)
}

fn rpc_max_response_size(&self) -> Result<Option<usize>> {
Ok(self.rpc_max_response_size)
}

fn rpc_max_subscriptions_per_connection(&self) -> Result<Option<usize>> {
Ok(self.rpc_max_subscriptions_per_connection)
}

fn ws_max_out_buffer_capacity(&self) -> Result<Option<usize>> {
Copy link
Contributor

@dvdplm dvdplm May 20, 2022

Choose a reason for hiding this comment

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

Add a deprecation eprintln! here too? And/or docs?

Copy link
Contributor Author

@niklasad1 niklasad1 May 20, 2022

Choose a reason for hiding this comment

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

https://github.com/paritytech/substrate/blob/8392760e00162cd01d95e4534a1996963b3d8e92/client/service/src/lib.rs#L482-#L526

Added it ☝️ here because it's annoying to print if one doesn't has applied that CLI option

Ok(self.ws_max_out_buffer_capacity)
}
Expand Down
7 changes: 6 additions & 1 deletion client/cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,11 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
Ok(None)
}

/// Get maximum number of subscriptions per connection.
Copy link
Contributor

@dvdplm dvdplm May 20, 2022

Choose a reason for hiding this comment

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

What does Ok(None) mean here? That it's using the default? If yes, maybe worth mentioning in the docs.

Copy link
Contributor Author

@niklasad1 niklasad1 May 20, 2022

Choose a reason for hiding this comment

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

Yes, it's a default imp for that CliConfiguration trait I just did the same as the other rpc options but clearly a footgun for
folks like me :(

fn rpc_max_subscriptions_per_connection(&self) -> Result<Option<usize>> {
Ok(None)
}

/// Get maximum WS output buffer capacity.
Copy link
Contributor

@dvdplm dvdplm May 20, 2022

Choose a reason for hiding this comment

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

This should be marked as deprecated maybe?

Copy link
Contributor Author

@niklasad1 niklasad1 May 20, 2022

Choose a reason for hiding this comment

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

It's already but added https://github.com/paritytech/substrate/blob/8392760e00162cd01d95e4534a1996963b3d8e92/client/service/src/lib.rs#L482-#L526

fn ws_max_out_buffer_capacity(&self) -> Result<Option<usize>> {
Ok(None)
Expand Down Expand Up @@ -539,7 +544,7 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
rpc_max_request_size: self.rpc_max_request_size()?,
rpc_max_response_size: self.rpc_max_response_size()?,
rpc_id_provider: None,
rpc_max_subs_per_conn: None,
rpc_max_subs_per_conn: self.rpc_max_subscriptions_per_connection()?,
ws_max_out_buffer_capacity: self.ws_max_out_buffer_capacity()?,
prometheus_config: self
.prometheus_config(DCV::prometheus_listen_port(), &chain_spec)?,
Expand Down
18 changes: 13 additions & 5 deletions client/service/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,11 +480,19 @@ where
}

fn legacy_cli_parsing(config: &Configuration) -> (Option<usize>, Option<usize>, Option<usize>) {
let ws_max_response_size = config.ws_max_out_buffer_capacity.map(|max| {
eprintln!("DEPRECATED: `--ws_max_out_buffer_capacity` has been removed use `rpc-max-response-size or rpc-max-request-size` instead");
eprintln!("Setting WS `rpc-max-response-size` to `max(ws_max_out_buffer_capacity, rpc_max_response_size)`");
std::cmp::max(max, config.rpc_max_response_size.unwrap_or(0))
});
let ws_max_response_size = match (
config.ws_max_out_buffer_capacity,
config.rpc_max_response_size,
) {
(Some(m1), Some(m2)) => {
eprintln!("DEPRECATED: `--ws_max_out_buffer_capacity` has been removed use `rpc-max-response-size or rpc-max-request-size` instead");
eprintln!("Setting WS `rpc-max-response-size` to `max(ws_max_out_buffer_capacity, rpc_max_response_size)`");
Some(std::cmp::max(m1, m2))
},
(Some(m), None) => Some(m),
(None, Some(m)) => Some(m),
_ => None,
};

let max_request_size = match (config.rpc_max_payload, config.rpc_max_request_size) {
(Some(legacy_max), max) => {
Expand Down