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

[http client]: refactor with "syncronous-like" design #156

Merged
merged 28 commits into from
Nov 16, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d637b8d
experimental
niklasad1 Nov 5, 2020
3581cce
ci(benches): sync and concurrent roundtrips
niklasad1 Nov 6, 2020
b278010
ci(benches): sync and concurrent roundtrips
niklasad1 Nov 6, 2020
10cfe8c
fix(nits)
niklasad1 Nov 6, 2020
cf12a2f
feat(http client): limit max request body size
niklasad1 Nov 6, 2020
0b569f7
test(http transport): request limit test
niklasad1 Nov 9, 2020
3e2827d
test(http client): add tests.
niklasad1 Nov 9, 2020
a453d99
fix typo
niklasad1 Nov 9, 2020
1740d2e
fix(benches): make it compile again.
niklasad1 Nov 10, 2020
dd72e6a
fix(ws example): revert unintentional change.
niklasad1 Nov 10, 2020
a904bc2
test(http client): subscription response on call.
niklasad1 Nov 10, 2020
5d06d5d
fix(cleanup)
niklasad1 Nov 10, 2020
6a43545
fix(benches): make it compile again.
niklasad1 Nov 10, 2020
a6d9493
Update src/client/http/transport.rs
niklasad1 Nov 11, 2020
c1ebc08
fix(http client): `&str` -> `AsRef<str>`
niklasad1 Nov 11, 2020
a3ae6ea
docs(client types): better docs for Mismatch type.
niklasad1 Nov 11, 2020
be70049
style: `Default::default` -> `HttpConfig::default`
niklasad1 Nov 11, 2020
19c6c9c
fix(http client): read body size from header.
niklasad1 Nov 11, 2020
58bb1cc
test(raw http): enable tests to works again.
niklasad1 Nov 12, 2020
d4c0bcc
style: cargo fmt
niklasad1 Nov 12, 2020
77a9fe5
benches: address grumbles
niklasad1 Nov 12, 2020
fee846f
feat(jsonrpc response/request): impl `Display`
niklasad1 Nov 12, 2020
0714a11
refactor(logging): use display impl
niklasad1 Nov 12, 2020
1929b08
Merge branch 'v2' into v2-http-client-syncronous-call-structure
niklasad1 Nov 13, 2020
a3d6355
Merge remote-tracking branch 'origin/v2' into v2-http-client-syncrono…
niklasad1 Nov 13, 2020
9ef8736
fix(http client): nits.
niklasad1 Nov 16, 2020
e1d7f3c
Update benches/benches.rs
niklasad1 Nov 16, 2020
abc81c6
fix bad merge.
niklasad1 Nov 16, 2020
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
4 changes: 2 additions & 2 deletions src/client/http/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ use crate::types::jsonrpc::{self, JsonValue};
use std::sync::atomic::{AtomicU64, Ordering};

/// Default maximum request body size (10 MB).
const DEFAULT_MAX_BODY_SIZE_TEN_MB: usize = 10 * 1024 * 1024;
const DEFAULT_MAX_BODY_SIZE_TEN_MB: u32 = 10 * 1024 * 1024;

/// HTTP configuration.
#[derive(Copy, Clone)]
pub struct HttpConfig {
/// Maximum request body size in bytes.
pub max_request_body_size: usize,
pub max_request_body_size: u32,
}

/// JSON-RPC HTTP Client that provides functionality to perform method calls and notifications.
Expand Down
80 changes: 57 additions & 23 deletions src/client/http/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use crate::types::jsonrpc;
use futures::StreamExt;
use thiserror::Error;

const CONTENT_TYPE_JSON: &str = "application/json";

/// HTTP Transport Client.
#[derive(Debug, Clone)]
pub struct HttpTransportClient {
Expand All @@ -18,43 +20,42 @@ pub struct HttpTransportClient {
/// HTTP client,
client: hyper::Client<hyper::client::HttpConnector>,
/// Configurable max request body size
max_request_body_size: usize,
max_request_body_size: u32,
Copy link
Member Author

Choose a reason for hiding this comment

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

2 GB max size, I changed because I don't like usize because it's implicit i.e. platform dependent.

Could be u64 as well but I think 2 GB should be sufficient some browsers limit to 2GB (e.g, Firefox)

}

impl HttpTransportClient {
/// Initializes a new HTTP client.
pub fn new(target: impl AsRef<str>, max_request_body_size: usize) -> Result<Self, Error> {
pub fn new(target: impl AsRef<str>, max_request_body_size: u32) -> Result<Self, Error> {
let target = url::Url::parse(target.as_ref()).map_err(|e| Error::Url(format!("Invalid URL: {}", e).into()))?;
if target.scheme() != "http" {
if target.scheme() == "http" {
Ok(HttpTransportClient { client: hyper::Client::new(), target, max_request_body_size })
} else {
return Err(Error::Url("URL scheme not supported, expects 'http'".into()));
};
Ok(HttpTransportClient { client: hyper::Client::new(), target, max_request_body_size })
}
}

/// Send request.
async fn send_request(&self, request: jsonrpc::Request) -> Result<hyper::Response<hyper::Body>, Error> {
log::debug!("send: {}", jsonrpc::to_string(&request).expect("request valid JSON; qed"));
let body = jsonrpc::to_vec(&request).map_err(|e| Error::Serialization(e))?;
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved

if body.len() > self.max_request_body_size {
if body.len() > self.max_request_body_size as usize {
return Err(Error::RequestTooLarge);
}

let req = hyper::Request::post(self.target.as_str())
.header(hyper::header::CONTENT_TYPE, hyper::header::HeaderValue::from_static("application/json"))
.header(hyper::header::CONTENT_TYPE, hyper::header::HeaderValue::from_static(CONTENT_TYPE_JSON))
.header(hyper::header::ACCEPT, hyper::header::HeaderValue::from_static(CONTENT_TYPE_JSON))
.body(From::from(body))
.expect("Uri and request headers are valid; qed");
.expect("URI and request headers are valid; qed");

let response = match self.client.request(req).await {
Ok(r) => r,
Err(err) => return Err(Error::Http(Box::new(err))),
};
let response = self.client.request(req).await.map_err(|e| Error::Http(Box::new(e)))?;

if !response.status().is_success() {
return Err(Error::RequestFailure { status_code: response.status().into() });
if response.status().is_success() {
Ok(response)
} else {
Err(Error::RequestFailure { status_code: response.status().into() })
}

Ok(response)
}

/// Send notification.
Expand All @@ -68,28 +69,51 @@ impl HttpTransportClient {
&self,
request: jsonrpc::Request,
) -> Result<jsonrpc::Response, Error> {

let response = self.send_request(request).await?;
let body_size = read_content_length(response.headers()).unwrap_or(0);
let mut body_fut: hyper::Body = response.into_body();

let mut body = Vec::new();
if body_size > self.max_request_body_size {
return Err(Error::RequestTooLarge);
}

let mut body = Vec::with_capacity(body_size as usize);
Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that this will be faster for big payloads that needs be re-allocated but
I haven't benchmarked this change for bigger payloads because the HttpServer hardcodes the max request size to 16384.

Thus, it's just a guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like with_capacity even if it turns out it doesn't change performance at all (I suspect it doesn't and that the compiler does the right thing anyway) because it communicates to the reader that we intended to use this for the body before doing it.


while let Some(chunk) = body_fut.next().await {
let chunk = chunk.map_err(|e| Error::Http(Box::new(e)))?;
if chunk.len() + body.len() > self.max_request_body_size {
if chunk.len() + body.len() > self.max_request_body_size as usize {
return Err(Error::RequestTooLarge);
}
body.extend_from_slice(&chunk);
}

// Note that we don't check the Content-Type of the request. This is deemed
// unnecessary, as a parsing error while happen anyway.
// TODO: use Response::from_json
let as_json: jsonrpc::Response = jsonrpc::from_slice(&body).map_err(Error::ParseError)?;
log::debug!("recv: {}", jsonrpc::to_string(&as_json).expect("request valid JSON; qed"));
Ok(as_json)
let response: jsonrpc::Response = jsonrpc::from_slice(&body).map_err(Error::ParseError)?;
log::debug!("recv: {}", jsonrpc::to_string(&response).expect("request valid JSON; qed"));
Ok(response)
}
}


// Read `content_length` from HTTP Header.
//
// Returns `Some(val)` if `content_length` contains exactly one value.
// None otherwise.
fn read_content_length(header: &hyper::header::HeaderMap) -> Option<u32> {
let values = header.get_all("content-length");
let mut iter = values.iter();
let content_length = iter.next()?;
if iter.next().is_some() {
return None;
}

// HTTP Content-Length indicates number of bytes in decimal.
let length = content_length.to_str().ok()?;
u32::from_str_radix(length, 10).ok()
}

/// Error that can happen during a request.
#[derive(Debug, Error)]
pub enum Error {
Expand Down Expand Up @@ -128,7 +152,7 @@ pub enum Error {

#[cfg(test)]
mod tests {
use super::{Error, HttpTransportClient};
use super::{Error, HttpTransportClient, read_content_length};
use crate::types::jsonrpc::{Call, Id, MethodCall, Params, Request, Version};

#[test]
Expand All @@ -154,4 +178,14 @@ mod tests {
let response = client.send_request(request).await.unwrap_err();
assert!(matches!(response, Error::RequestTooLarge));
}

#[test]
fn read_content_length_works() {
let mut header = hyper::header::HeaderMap::new();
header.insert(hyper::header::CONTENT_LENGTH, "177".parse().unwrap());
assert_eq!(read_content_length(&header), Some(177));

header.append(hyper::header::CONTENT_LENGTH, "999".parse().unwrap());
assert_eq!(read_content_length(&header), None);
}
}