refactor: update and turn `WsTransportClientBuilder` generic #1168

oleonardolima · 2023-08-07T03:07:37Z

e7ecb9d refactor(jsonrpsee-client-transport): update and turn WsTransportClientBuilder generic

fix(docs): typo on WsTransportClientBuilder doc on Receiver
reference.
refactor: add initial fn signatures for build_with_stream,
try_connect_over_tcp, and try_connect.
refactor: expose EitherStream visibility to public.
refactor: make Sender and Receiver generic over T, a data stream.
refactor: make TransportSenderT and TransportReceiverT
implementations over generic Sender and Receiver, bound to
AsyncRead, AsyncRead, MaybeSend and 'static.
refactor: turn old try_connect TCP steps into
try_connect_over_tcp.
feat: implement build_with_stream and try_connect to handle and
handle the handshake for a generic data stream T.
feat: add new Redirected error variant to WsHandshakeError, as it
should be handled by the client when using a generic data stream T.

c7ae7dd refactor(jsonrpsee-ws-client): add new fns to WsClientBuilder

feat: add new WsClientBuilder::build_with_transport that builds and
returns a WsClient with the given Sender and Receiver.
feat: add new WsClientBuilder::build_with_stream that uses the new
WsTransportClientBuilder::build_with_stream, building and returning
the WsClient with the given data_stream as transport layer.
refactor: update the WsClientBuilder::build to use the new
build_with_transport, it helps not having duplicated code.

10e78a0 refactor: re-export EitherStream & sort current list

53641d1 test: add integration tests and helper fns

add new helper fns to spawn a socks5 server, using fast-socks5
add new helper enum for DataStream that acts as a wrapper to
Socks5Stream<T>, similar to what a client would need to do.
impl AsyncRead + AsyncWrite for the helper DataStream enum, to make
it compatible between futures::io and tokio::io.
add new tests that connects over a socks5 proxy, and use the new
`WsClientBuilder::default()::build_with_stream(...) fn.

fixes #1162 #870

niklasad1 · 2023-10-11T09:39:20Z

@oleonardolima any update on this one?

Let us know whether you need help to push this further.

oleonardolima · 2023-10-18T02:04:43Z

@niklasad1 I plan to get back to it by the end of the week. Sorry for the delayed PR, conferences and changing jobs got me delayed on it.

oleonardolima · 2023-10-20T01:10:43Z

@oleonardolima any update on this one?

Let us know whether you need help to push this further.

@niklasad1 Do you see any problem with adding the arti-client dependency on jsonrpsee-integration-tests in order to have integration tests for different streams other than tokio::net::TcpStream ?

client/transport/src/ws/mod.rs

proc-macros/tests/ui/incorrect/rpc/rpc_deprecated_method.stderr

niklasad1 · 2023-10-20T08:57:08Z

@niklasad1 Do you see any problem with adding the arti-client dependency on jsonrpsee-integration-tests in order to have integration tests for different streams other than tokio::net::TcpStream ?

It's not possible to use a more light-weight dependency than arti-client for that?

I'm happy if we just have a test to wrap another stream using the WsTransportBuilder::build_with_stream and assert connecting and make a simple RPC call works.

client/transport/src/ws/mod.rs

client/ws-client/src/lib.rs

niklasad1

Yupp, nice looks you are on the right path.

This PR just needs to fix a few minor things + add few tests then it should be good to go.

oleonardolima · 2023-10-23T23:21:28Z

It's not possible to use a more light-weight dependency than arti-client for that?

I'm happy if we just have a test to wrap another stream using the WsTransportBuilder::build_with_stream and assert connecting and make a simple RPC call works.

Sure, I'll check if there is a more light-weight one.

PS.: We can use the TcpStream, but wouldn't be testing anything new in my opinion (because is the default already used one)

niklasad1 · 2023-10-24T07:28:51Z

PS.: We can use the TcpStream, but wouldn't be testing anything new in my opinion (because is the default already used one)

Yeah, I agree that we should test with another one

niklasad1 · 2023-11-15T08:50:19Z

tests/tests/helpers.rs

+
+#[pin_project(project = DataStreamProj)]
+#[allow(dead_code)]
+pub enum DataStream<T: tokio::io::AsyncRead + tokio::io::AsyncWrite + std::marker::Unpin> {


I don't understand why this type is needed, can't you just use fast_socks5::client::Socks5Stream directly instead because it already implements AsyncRead and AsyncWrite?

@niklasad1 It does implement the tokio::io::AsyncRead and tokio::io::AsyncWrite, but currently the TransportSenderT and TransportReceiverT requires the futures::io::AsyncRead and futures::io::AsyncRead implementation.

I made this way not to change the current implementation of EitherStream, which implements the futures::io traits.

I can change the EitherStream to use the tokio::io traits instead, do you see any problem ?

I'm actually biased because requiring the tokio traits as default would make it easier for me on other projects 😅

This is just for tests so I don't mind having this extra code.

I can change the EitherStream to use the tokio::io traits instead, do you see any problem ?

Let's keep it out this PR but tokio::io makes sense to me since this crate it's tightly-coupled to tokio nowdays.
This is probably old code when this was supposed to support both tokio and async-std.

//cc @lexnv @jsdw

Ok, I can give it a try on another PR as well, as tokio "native" support will help me with changes on the fedimint side

Agreed that moving to tokio::io traits probably makes sense and might simplify some stuff given that we are "all in" on tokio in this lib nowadays

Also I wonder why this is an enum and not struct?

yeah, would be cleaner with a struct to avoid matching on it :P

Great, thanks for the suggestion and @niklasad1 for addressing the changes

niklasad1

Nice, looks good

Run cargo fmt --all to make the CI happy.

niklasad1

Looks good to me, nice work

Can you please run cargo clippy and fix the warnings?

@oleonardolima

…entBuilder` generic - fix(docs): typo on `WsTransportClientBuilder` doc on `Receiver` reference. - refactor: add initial fn signatures for `build_with_stream`, `try_connect_over_tcp`, and `try_connect`. - refactor: expose `EitherStream` visibility to public. - refactor: make `Sender` and `Receiver` generic over T, a data stream. - refactor: make `TransportSenderT` and `TransportReceiverT` implementations over generic `Sender` and `Receiver`, bound to `AsyncRead`, `AsyncRead`, `MaybeSend` and `'static`. - refactor: turn old `try_connect` TCP steps into `try_connect_over_tcp`. - feat: implement `build_with_stream` and `try_connect` to handle and handle the handshake for a generic data stream `T`. - feat: add new `Redirected` error variant to `WsHandshakeError`, as it should be handled by the client when using a generic data stream `T`. - TODO(@oleonardolima): Add new tests that uses a different data stream.

- feat: add new `WsClientBuilder::build_with_transport` that builds and returns a `WsClient` with the given `Sender` and `Receiver`. - feat: add new `WsClientBuilder::build_with_stream` that uses the new `WsTransportClientBuilder::build_with_stream`, building and returning the `WsClient` with the given `data_stream` as transport layer. - refactor: update the `WsClientBuilder::build` to use the new `build_with_transport`, it helps not having duplicated code.

- add new helper fns to spawn a socks5 server, using `fast-socks5` - add new helper enum for `DataStream` that acts as a wrapper to `Socks5Stream<T>`, similar to what a client would need to do. - impl AsyncRead + AsyncWrite for the helper `DataStream` enum, to make it compatible between futures::io and tokio::io. - add new tests that connects over a socks5 proxy, and use the new `WsClientBuilder::default()::build_with_stream(...) fn.

oleonardolima · 2023-11-20T19:02:19Z

Looks good to me, nice work

Can you please run cargo clippy and fix the warnings?

Sure, CI is all good now 🚀

jsdw · 2023-11-21T10:37:49Z

client/transport/src/ws/mod.rs

 					}
 				};
 			}
 		}
 		err.unwrap_or(Err(WsHandshakeError::NoAddressFound(target.host)))
 	}
+
+	/// Try to establish the connection over the given data stream.
+	pub async fn build_with_stream<T>(


Nit: I'd move this fn to be right underneath build

jsdw · 2023-11-21T11:03:24Z

tests/tests/helpers.rs

@@ -249,3 +254,112 @@ pub async fn pipe_from_stream_and_drop<T: Serialize>(
 		}
 	}
 }
+
+#[allow(dead_code)]


I'd have to compile this to check but I wonder why all the #[allow(dead_code)]s are needed!

I don't remember but probably that is just used for tests are not exported outside the crate.

Maybe pub(crate) would do it...

If they are only used for tests then having the helpers be behind #[cfg(test)] would also remove such a warning :)

we don't have a lib.rs in the crate it's like https://github.com/paritytech/jsonrpsee/tree/master/tests/tests but every file should #![cfg(test)] so I don't know ^^

Sorry about that, totally missed it

tests/tests/helpers.rs

jsdw · 2023-11-21T11:11:24Z

tests/tests/integration_tests.rs

+	let mut success = false;
+
+	// Wait until a slot is available, as only one concurrent call is allowed.
+	// Then when this finishes we know that unsubscribe call has been finished.
+	for _ in 0..30 {
+		let res: Result<String, _> = client.request("say_hello", rpc_params![]).await;
+		if res.is_ok() {
+			success = true;
+			break;
+		}
+		tokio::time::sleep(std::time::Duration::from_millis(100)).await;
+	}


So this is all just here to check that unsubscribe has finished; I wonder whether there is a simpler/cleaner way to do this (@niklasad1)

yeah but I realized that the subscription doesn't keep the request id guard so this doesn't work as intended.

let's remove the extra ugliness for this test when this PR is merged...

jsdw

Looks like a nice addition to me; thank you! Just some nits but nothing super important

oleonardolima force-pushed the refactor/update-ws-client-transport-to-generic-over-stream branch from cdc6ce4 to 7985210 Compare August 23, 2023 00:39

justinmoon mentioned this pull request Aug 28, 2023

Tor Support Tracking Issue fedimint/fedimint#2610

Open

14 tasks

oleonardolima force-pushed the refactor/update-ws-client-transport-to-generic-over-stream branch 4 times, most recently from 9a08f03 to c7ae7dd Compare October 20, 2023 01:08

oleonardolima changed the title ~~wip(refactor): update and turn WsTransportClientBuilder generic~~ refactor: update and turn WsTransportClientBuilder generic Oct 20, 2023

oleonardolima force-pushed the refactor/update-ws-client-transport-to-generic-over-stream branch from c7ae7dd to 988bd1e Compare October 20, 2023 02:52