Client http transport #420

ascjones · 2019-04-16T09:30:46Z

Still todo:

Improve ergonomics of channel impl - helper for the transport Future
- Use a combinator e.g. join or wrap in a struct which impls Future
Tests/examples of http client in use
Fix error handling in core client, channels shouldn't be dropped because of rpc errors

ascjones · 2019-05-02T15:11:57Z

I've changed my approach here: initially I copied @tomusdrw's approach of creating a pair of channels for the Sink/Stream and creating the client with those.

jsonrpc/core-client/src/transports/http.rs

Lines 17 to 33 in 7a1c1f4

    
           let (rpc_client, sender) = request_response(8, move |request: String| { 
        
           	let request = Request::post(&url) 
        
           		.header(http::header::CONTENT_TYPE, http::header::HeaderValue::from_static("application/json")) 
        
           		.body(request.into()) 
        
           		.unwrap(); 
        
           	client 
        
           		.request(request) 
        
           		.map_err(|e| RpcError::Other(e.into())) 
        
           		.and_then(|res| { 
        
           			// TODO [ToDr] Handle non-200 
        
           			res.into_body() 
        
           				.map_err(|e| RpcError::ParseError(e.to_string(), e.into())) 
        
           				.concat2() 
        
           				.map(|chunk| String::from_utf8_lossy(chunk.as_ref()).into_owned()) 
        
           		}) 
        
           });

jsonrpc/core-client/src/transports/mod.rs

Lines 10 to 40 in 7a1c1f4

    
           pub fn request_response<F, R>(request_buffer: usize, f: F) -> ( 
        
           	impl Future<Item=(), Error=RpcError>, 
        
           	RpcChannel, 
        
           ) where 
        
           	F: Fn(String) -> R, 
        
           	R: Future<Item=String, Error=RpcError>, 
        
           { 
        
           	let (requests, requests_rx) = mpsc::channel(request_buffer); 
        
           	let (responses, responses_rx) = mpsc::channel(0); 
        
           	let run= requests_rx 
        
           		.map(f) 
        
           		.forward(responses.sink_map_err(|_e| ())) 
        
           		.map(|_| ()); 
        
           	let sink = requests 
        
           		.sink_map_err(|e| RpcError::Other(e.into())); 
        
           	let stream = responses_rx 
        
           		.map_err(|()| unreachable!()) 
        
           		.and_then(|res| res); 
        
           	let (rpc_client, sender) = RpcClient::with_channel(sink, stream); 
        
           	let client = run 
        
           		.map_err(|()| RpcError::Other(format_err!("Transport error"))) // todo: [AJ] check unifying of error types 
        
           		.join(rpc_client) 
        
           		.map(|((), ())| ()); 
        
           	(client, sender) 
        
           }

However this ran into problems where HTTP errors would not be returned to the caller, because there is no way to correlate the error response with the sender in RpcClient, since the request Id is only available in a successful response from the server.

So I've created a "standalone" HTTP transport, which can handle the errors properly since it is request/response instead of duplex.

I've extracted the former RpcClient into transports and renamed it Duplex. Shared functionality between that and the HTTP transport (id assignment, request building. response handling) I've extracted out into some helpers.

Still a bit more work to do on this - but would be good to have some feedback to see whether this is the right direction.

dvc94ch · 2019-05-03T14:37:49Z

However this ran into problems where HTTP errors would not be returned to the caller, because there is no way to correlate the error response with the sender in RpcClient, since the request Id is only available in a successful response from the server.

What happens currently? Is the error at least logged?

I see it's logged in the ws client which is suboptimal.

ascjones · 2019-05-03T14:45:12Z

I think it would only be logged if you attach a logger to the client future error e.g. https://github.com/paritytech/substrate/pull/2255/files#diff-74c6a3e10a271935aa30a9981463e681R76

* Run rustfmt. * Make RpcMessage private. * Move ws/client to core-client.

tomusdrw

Looks good after the first glance, couple of small nits.

core-client/Cargo.toml

tomusdrw · 2019-05-20T08:12:49Z

core-client/src/lib.rs

-			channel: Some(channel),
-			outgoing: VecDeque::new(),
-		}
+impl Into<RpcChannel> for mpsc::Sender<RpcMessage> {


I think we should rather implement From:

Library authors should not directly implement this[Into] trait, but should prefer implementing the From trait, which offers greater flexibility and provides an equivalent Into implementation for free, thanks to a blanket implementation in the standard library.

tomusdrw · 2019-05-20T08:15:58Z