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.

Sign up for GitHub

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

Jump to bottom

refactor: set tcp_nodelay == true by default and explicit APIs #1263

Merged
merged 7 commits into from
Dec 11, 2023
Merged

refactor: set tcp_nodelay == true by default and explicit APIs #1263

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
36f440e
feat: add explicit API `set_tcp_nodelay`
niklasad1 Dec 11, 2023
3c7b731
fix build
niklasad1 Dec 11, 2023
3945160
replace messy fn new with builder
niklasad1 Dec 11, 2023
3bfffb8
update changelog
niklasad1 Dec 11, 2023
453bc30
Update client/http-client/src/transport.rs
niklasad1 Dec 11, 2023
7fe98cc
fix bad fmt
niklasad1 Dec 11, 2023
6aa5595
Merge remote-tracking branch 'origin/na-fix-1262' into na-fix-1262
niklasad1 Dec 11, 2023
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
15 changes: 14 additions & 1 deletion client/http-client/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use tower::layer::util::Identity;
use tower::{Layer, Service};
use tracing::instrument;

/// Http Client Builder.
/// HttpClient Builder.
///
/// # Examples
///
Expand Down Expand Up @@ -83,6 +83,7 @@ pub struct HttpClientBuilder<L = Identity> {
max_log_length: u32,
headers: HeaderMap,
service_builder: tower::ServiceBuilder<L>,
tcp_no_delay: bool,
}

impl<L> HttpClientBuilder<L> {
Expand Down Expand Up @@ -160,6 +161,14 @@ impl<L> HttpClientBuilder<L> {
self
}

/// Configure `TCP_NODELAY` on the socket to the supplied value `nodelay`.
///
/// Default is `true`.
pub fn set_tcp_no_delay(mut self, no_delay: bool) -> Self {
self.tcp_no_delay = no_delay;
self
}

/// Set custom tower middleware.
pub fn set_http_middleware<T>(self, service_builder: tower::ServiceBuilder<T>) -> HttpClientBuilder<T> {
HttpClientBuilder {
Expand All @@ -172,6 +181,7 @@ impl<L> HttpClientBuilder<L> {
max_response_size: self.max_response_size,
service_builder,
request_timeout: self.request_timeout,
tcp_no_delay: self.tcp_no_delay,
}
}
}
Expand All @@ -196,6 +206,7 @@ where
headers,
max_log_length,
service_builder,
tcp_no_delay,
..
} = self;

Expand All @@ -207,6 +218,7 @@ where
max_log_length,
headers,
service_builder,
tcp_no_delay,
)
.map_err(|e| Error::Transport(e.into()))?;
Ok(HttpClient {
Expand All @@ -229,6 +241,7 @@ impl Default for HttpClientBuilder<Identity> {
max_log_length: 4096,
headers: HeaderMap::new(),
service_builder: tower::ServiceBuilder::new(),
tcp_no_delay: true,
}
}
}
Expand Down
32 changes: 27 additions & 5 deletions client/http-client/src/transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,16 @@
B::Error: Into<Box<dyn StdError + Send + Sync>>,
{
/// Initializes a new HTTP client.
pub(crate) fn new<L: Layer<HttpBackend<Body>, Service = S>>(
max_request_size: u32,
target: impl AsRef<str>,
max_response_size: u32,
cert_store: CertificateStore,
max_log_length: u32,
headers: HeaderMap,
service_builder: tower::ServiceBuilder<L>,
tcp_no_delay: bool,
) -> Result<Self, Error> {

Check warning on line 111 in client/http-client/src/transport.rs

View workflow job for this annotation

GitHub Actions / clippy

this function has too many arguments (8/7) 

warning: this function has too many arguments (8/7)
   --> client/http-client/src/transport.rs:102:2
    |
102 | /     pub(crate) fn new<L: Layer<HttpBackend<Body>, Service = S>>(
103 | |         max_request_size: u32,
104 | |         target: impl AsRef<str>,
105 | |         max_response_size: u32,
...   |
110 | |         tcp_no_delay: bool,
111 | |     ) -> Result<Self, Error> {
    | |____________________________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
    = note: `#[warn(clippy::too_many_arguments)]` on by default
let mut url = Url::parse(target.as_ref()).map_err(|e| Error::Url(format!("Invalid URL: {e}")))?;
if url.host_str().is_none() {
return Err(Error::Url("Invalid host".into()));
Expand All @@ -115,25 +116,34 @@
url.set_fragment(None);

let client = match url.scheme() {
"http" => HttpBackend::Http(Client::new()),
"http" => {
let mut connector = HttpConnector::new();
connector.set_nodelay(tcp_no_delay);
HttpBackend::Http(Client::builder().build(connector))
}
#[cfg(feature = "__tls")]
"https" => {
let connector = match cert_store {
let mut http_conn = HttpConnector::new();
http_conn.set_nodelay(tcp_no_delay);
http_conn.enforce_http(false);

let https_conn = match cert_store {
#[cfg(feature = "native-tls")]
CertificateStore::Native => hyper_rustls::HttpsConnectorBuilder::new()
.with_native_roots()
.https_or_http()
.enable_all_versions()
.build(),
.wrap_connector(http_conn),
#[cfg(feature = "webpki-tls")]
CertificateStore::WebPki => hyper_rustls::HttpsConnectorBuilder::new()
.with_webpki_roots()
.https_or_http()
.enable_all_versions()
.build(),
.wrap_connector(http_conn),
_ => return Err(Error::InvalidCertficateStore),
};
HttpBackend::Https(Client::builder().build::<_, hyper::Body>(connector))

HttpBackend::Https(Client::builder().build::<_, hyper::Body>(https_conn))
}
_ => {
#[cfg(feature = "__tls")]
Expand Down Expand Up @@ -268,6 +278,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap_err();
assert!(matches!(err, Error::Url(_)));
Expand All @@ -284,6 +295,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "https://localhost/");
Expand All @@ -300,6 +312,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap_err();
assert!(matches!(err, Error::Url(_)));
Expand All @@ -315,6 +328,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap_err();
assert!(matches!(err, Error::Url(_)));
Expand All @@ -326,6 +340,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap_err();
assert!(matches!(err, Error::Url(_)));
Expand All @@ -341,6 +356,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "http://localhost/my-special-path");
Expand All @@ -356,6 +372,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "http://127.0.0.1/my?name1=value1&name2=value2");
Expand All @@ -371,6 +388,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "http://127.0.0.1/my.htm");
Expand All @@ -386,6 +404,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "http://127.0.0.1/");
Expand All @@ -402,6 +421,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "https://localhost:9999/");
Expand All @@ -417,6 +437,7 @@
80,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(&client.target, "http://localhost:9999/");
Expand All @@ -435,6 +456,7 @@
99,
HeaderMap::new(),
tower::ServiceBuilder::new(),
true,
)
.unwrap();
assert_eq!(client.max_request_size, eighty_bytes_limit);
Expand Down
10 changes: 8 additions & 2 deletions client/transport/src/ws/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ pub struct WsTransportClientBuilder {
pub max_response_size: u32,
/// Max number of redirections.
pub max_redirections: usize,
/// TCP no delay.
pub tcp_no_delay: bool,
}

impl Default for WsTransportClientBuilder {
Expand All @@ -89,6 +91,7 @@ impl Default for WsTransportClientBuilder {
connection_timeout: Duration::from_secs(10),
headers: http::HeaderMap::new(),
max_redirections: 5,
tcp_no_delay: true,
}
}
}
Expand Down Expand Up @@ -338,7 +341,9 @@ impl WsTransportClientBuilder {
let sockaddrs = std::mem::take(&mut target.sockaddrs);
for sockaddr in &sockaddrs {
#[cfg(feature = "__tls")]
let tcp_stream = match connect(*sockaddr, self.connection_timeout, &target.host, connector.as_ref()).await {
let tcp_stream = match connect(*sockaddr, self.connection_timeout, &target.host, connector.as_ref(), self.tcp_no_delay)
.await
{
Ok(stream) => stream,
Err(e) => {
tracing::debug!(target: LOG_TARGET, "Failed to connect to sockaddr: {:?}", sockaddr);
Expand Down Expand Up @@ -469,13 +474,14 @@ async fn connect(
timeout_dur: Duration,
host: &str,
tls_connector: Option<&tokio_rustls::TlsConnector>,
tcp_no_delay: bool,
) -> Result<EitherStream, WsHandshakeError> {
let socket = TcpStream::connect(sockaddr);
let timeout = tokio::time::sleep(timeout_dur);
tokio::select! {
socket = socket => {
let socket = socket?;
if let Err(err) = socket.set_nodelay(true) {
if let Err(err) = socket.set_nodelay(tcp_no_delay) {
tracing::warn!(target: LOG_TARGET, "set nodelay failed: {:?}", err);
}
match tls_connector {
Expand Down
14 changes: 13 additions & 1 deletion client/ws-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ pub struct WsClientBuilder {
max_redirections: usize,
id_kind: IdKind,
max_log_length: u32,
tcp_no_delay: bool,
}

impl Default for WsClientBuilder {
Expand All @@ -107,6 +108,7 @@ impl Default for WsClientBuilder {
max_redirections: 5,
id_kind: IdKind::Number,
max_log_length: 4096,
tcp_no_delay: true,
}
}
}
Expand Down Expand Up @@ -221,6 +223,12 @@ impl WsClientBuilder {
self
}

/// See documentation [`ClientBuilder::set_tcp_no_delay`] (default is true).
pub fn set_tcp_no_delay(mut self, no_delay: bool) -> Self {
self.tcp_no_delay = no_delay;
self
}

/// Build the [`WsClient`] with specified [`TransportSenderT`] [`TransportReceiverT`] parameters
///
/// ## Panics
Expand All @@ -238,6 +246,7 @@ impl WsClientBuilder {
max_buffer_capacity_per_subscription,
id_kind,
max_log_length,
tcp_no_delay,
..
} = self;

Expand All @@ -246,7 +255,8 @@ impl WsClientBuilder {
.request_timeout(request_timeout)
.max_concurrent_requests(max_concurrent_requests)
.id_format(id_kind)
.set_max_logging_length(max_log_length);
.set_max_logging_length(max_log_length)
.set_tcp_no_delay(tcp_no_delay);

if let Some(cfg) = ping_config {
client = client.enable_ws_ping(cfg);
Expand All @@ -271,6 +281,7 @@ impl WsClientBuilder {
max_request_size: self.max_request_size,
max_response_size: self.max_response_size,
max_redirections: self.max_redirections,
tcp_no_delay: self.tcp_no_delay,
};

let uri = Url::parse(url.as_ref()).map_err(|e| Error::Transport(e.into()))?;
Expand All @@ -295,6 +306,7 @@ impl WsClientBuilder {
max_request_size: self.max_request_size,
max_response_size: self.max_response_size,
max_redirections: self.max_redirections,
tcp_no_delay: self.tcp_no_delay,
};

let uri = Url::parse(url.as_ref()).map_err(|e| Error::Transport(e.into()))?;
Expand Down
12 changes: 12 additions & 0 deletions core/src/client/async_client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ pub struct ClientBuilder {
id_kind: IdKind,
max_log_length: u32,
ping_config: Option<PingConfig>,
tcp_no_delay: bool,
}

impl Default for ClientBuilder {
Expand All @@ -226,6 +227,7 @@ impl Default for ClientBuilder {
id_kind: IdKind::Number,
max_log_length: 4096,
ping_config: None,
tcp_no_delay: true,
}
}
}
Expand Down Expand Up @@ -296,6 +298,16 @@ impl ClientBuilder {
self
}

/// Configure `TCP_NODELAY` on the socket to the supplied value `nodelay`.
///
/// On some transports this may have no effect.
///
/// Default is `true`.
pub fn set_tcp_no_delay(mut self, no_delay: bool) -> Self {
self.tcp_no_delay = no_delay;
self
}

/// Build the client with given transport.
///
/// ## Panics
Expand Down
13 changes: 12 additions & 1 deletion server/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ pub struct ServerConfig {
pub(crate) ping_config: Option<PingConfig>,
/// ID provider.
pub(crate) id_provider: Arc<dyn IdProvider>,
/// `TCP_NODELAY` settings.
pub(crate) tcp_no_delay: bool,
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -350,6 +352,7 @@ impl Default for ServerConfig {
message_buffer_capacity: 1024,
ping_config: None,
id_provider: Arc::new(RandomIntegerIdProvider),
tcp_no_delay: true,
}
}
}
Expand Down Expand Up @@ -725,6 +728,14 @@ impl<HttpMiddleware, RpcMiddleware> Builder<HttpMiddleware, RpcMiddleware> {
Builder { server_cfg: self.server_cfg, http_middleware, rpc_middleware: self.rpc_middleware }
}

/// Configure `TCP_NODELAY` on the socket to the supplied value `nodelay`.
///
/// Default is `true`.
pub fn set_tcp_no_delay(mut self, no_delay: bool) -> Self {
self.server_cfg.tcp_no_delay = no_delay;
self
}

/// Configure the server to only serve JSON-RPC HTTP requests.
///
/// Default: both http and ws are enabled.
Expand Down Expand Up @@ -1151,7 +1162,7 @@ fn process_connection<'a, RpcMiddleware, HttpMiddleware, U>(
..
} = params;

if let Err(e) = socket.set_nodelay(true) {
if let Err(e) = socket.set_nodelay(server_cfg.tcp_no_delay) {
tracing::warn!(target: LOG_TARGET, "Could not set NODELAY on socket: {:?}", e);
return;
}
Expand Down
Loading