[ws client]: add `wss` test + refactor `WebSocketTransport builder` #209

niklasad1 · 2021-02-12T16:58:39Z

No description provided.

ws-client/src/transport.rs

dvdplm · 2021-02-14T13:57:38Z

tests/src/lib.rs

@@ -207,3 +207,10 @@ async fn ws_more_request_than_buffer_should_not_deadlock() {
 		req.await.unwrap();
 	}
 }
+
+#[tokio::test]
+async fn wss_works() {


Would be good to be complete here:

assert that ws:// works

assert that omitting the port works (and uses the default)

assert that providing a wacky port fails, bigger than u16::MAX, negative number, 0 etc

assert that host name must be ascii (does it?)

ws-client/src/transport.rs

niklasad1 · 2021-02-16T12:03:59Z

ws-client/src/transport.rs

@@ -37,6 +37,15 @@ use thiserror::Error;

 type TlsOrPlain = crate::stream::EitherStream<TcpStream, TlsStream<TcpStream>>;

+#[derive(Clone)]
+pub struct Host(String);


Introduced a new type to avoid nasty bugs when several params has the same type.

It could be a pub struct Host<'a>(&'a str)

I think String is fine.

niklasad1 · 2021-02-16T13:33:39Z

@dvdplm I think this is ready for another round of review :)

niklasad1 · 2021-02-17T10:21:03Z

tests/src/lib.rs

+
+#[tokio::test]
+async fn http_with_non_ascii_url_doesnt_hang_or_panic() {
+	let client = HttpClient::new("http://♥♥♥♥♥♥∀∂", HttpConfig::default()).unwrap();


https://docs.rs/url/2.2.0/url/enum.Host.html#variant.Domain is the reason why it's doesn't fail parsing the URL in the constructor.

and in the WebSocket case the host is resolved to a SockAddr because an actual connection is established in the constructor.

niklasad1 · 2021-02-17T12:30:24Z

ws-client/src/transport.rs

+			_ => return Err(WsHandshakeError::Url("URL scheme not supported, expects 'ws' or 'wss'".into())),
+		};
+		let host = url.host_str().ok_or_else(|| WsHandshakeError::Url("No host in URL".into()))?.into();
+		let sockaddrs: Vec<SocketAddr> = url.socket_addrs(|| None).map_err(WsHandshakeError::ResolutionFailed)?;


switched to https://docs.rs/url/2.2.0/url/struct.Url.html#method.socket_addrs instead which provides the default port number if it's missing such that we don't have to do it manually anymore

Do you think we should add a comment stating this and what the default port is?

I think the best option is the document this on WsConfig i.e, default ports is ws - 80, wss - 433 ^^

Here we could add a comment that sock_addrs() uses https://docs.rs/url/2.2.0/url/struct.Url.html#method.port_or_known_default if the port is missing

niklasad1 · 2021-02-17T12:32:16Z

ws-client/src/transport.rs

@@ -146,43 +159,8 @@ pub enum WsConnectError {

 /// Creates a new WebSocket connection based on [`WsConfig`](crate::WsConfig) represented as a Sender and Receiver pair.
 pub async fn websocket_connection(config: WsConfig<'_>) -> Result<(Sender, Receiver), WsHandshakeError> {


if you like the change i.e, provide TryFrom<WsConfig> for WebSocketTransportClient { ... } then I think we could remove this function.

insipx

LGTM.

dvdplm

A few minor nits.

dvdplm · 2021-02-18T08:32:41Z

tests/src/lib.rs

+
+#[tokio::test]
+async fn http_with_non_ascii_url_doesnt_hang_or_panic() {
+	let client = HttpClient::new("http://♥♥♥♥♥♥∀∂", HttpConfig::default()).unwrap();


dvdplm · 2021-02-18T08:33:04Z

ws-client/src/transport.rs

@@ -37,6 +37,15 @@ use thiserror::Error;

 type TlsOrPlain = crate::stream::EitherStream<TcpStream, TlsStream<TcpStream>>;

+#[derive(Clone)]
+pub struct Host(String);


I think String is fine.

dvdplm · 2021-02-18T08:36:26Z

ws-client/src/transport.rs

+			_ => return Err(WsHandshakeError::Url("URL scheme not supported, expects 'ws' or 'wss'".into())),
+		};
+		let host = url.host_str().ok_or_else(|| WsHandshakeError::Url("No host in URL".into()))?.into();
+		let sockaddrs: Vec<SocketAddr> = url.socket_addrs(|| None).map_err(WsHandshakeError::ResolutionFailed)?;


Do you think we should add a comment stating this and what the default port is?

dvdplm · 2021-02-18T08:40:05Z

ws-client/src/transport.rs

+			"wss" => Mode::Tls,
+			_ => return Err(WsHandshakeError::Url("URL scheme not supported, expects 'ws' or 'wss'".into())),
+		};
+		let host = url.host_str().ok_or_else(|| WsHandshakeError::Url("No host in URL".into()))?.into();


Isn't the error case taken care of by the url::Url::parse() call on line 276? Oh, I see, it is None for unix sockets... Ok, fine then.

The error is bit funny though, it's not really a handshaking error. Can we use a more appropriate error?

The error is bit funny though, it's not really a handshaking error. Can we use a more appropriate error?

indeed, let's fix that in another PR.

#217

dvdplm · 2021-02-18T08:41:03Z

ws-client/src/transport.rs

+impl<'a> TryFrom<WsConfig<'a>> for WsTransportClientBuilder<'a> {
+	type Error = WsHandshakeError;
+
+	fn try_from(config: WsConfig<'a>) -> Result<Self, Self::Error> {


I quite like this pattern of converting a config into a builder.

niklasad1 added 2 commits February 12, 2021 17:56

test(ws client): wss (TLS)

1525e65

fix(ws client): bug replace target with host.

32c53a3

niklasad1 requested review from dvdplm, insipx and maciejhirsz February 12, 2021 16:59

niklasad1 commented Feb 12, 2021

View reviewed changes

ws-client/src/transport.rs Outdated Show resolved Hide resolved

dvdplm suggested changes Feb 14, 2021

View reviewed changes

niklasad1 added 2 commits February 16, 2021 10:13

Merge remote-tracking branch 'origin/master' into ws-client-add-wss-test

0d84a1c

address review suggestions

5b6ff85

niklasad1 commented Feb 16, 2021

View reviewed changes

ws-client/src/transport.rs Outdated Show resolved Hide resolved

niklasad1 commented Feb 16, 2021

View reviewed changes

tests: add tests for non-ascii URLs

bfd4ccd

niklasad1 commented Feb 17, 2021

View reviewed changes

niklasad1 added 2 commits February 17, 2021 13:08

refactor: impl TryFrom for WebSocketTransportBuild

f7de4b7

fix nit: try to connect to all sockaddrs

97bedb3

niklasad1 commented Feb 17, 2021

View reviewed changes

insipx approved these changes Feb 17, 2021

View reviewed changes

niklasad1 changed the title ~~[ws client]: add wss test~~ [ws client]: add wss test + refactor WebSocketTransport builder Feb 17, 2021

document Host type

af9c521

dvdplm approved these changes Feb 18, 2021

View reviewed changes

niklasad1 mentioned this pull request Feb 18, 2021

[ws client transport]: refactor errors #217

Closed

niklasad1 added 2 commits February 18, 2021 10:17

doc(ws client): default port.

e51bdc2

remove helper function

4b98f9e

niklasad1 merged commit 1cdd138 into master Feb 18, 2021

niklasad1 deleted the ws-client-add-wss-test branch February 18, 2021 11:01

niklasad1 mentioned this pull request Mar 30, 2021

Error: TransportError(NoAddressFound) #255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ws client]: add `wss` test + refactor `WebSocketTransport builder` #209

[ws client]: add `wss` test + refactor `WebSocketTransport builder` #209

niklasad1 commented Feb 12, 2021

dvdplm Feb 14, 2021

niklasad1 Feb 16, 2021 •

edited

Loading

dvdplm Feb 18, 2021

niklasad1 commented Feb 16, 2021

niklasad1 Feb 17, 2021

niklasad1 Feb 17, 2021

dvdplm Feb 18, 2021

niklasad1 Feb 17, 2021

dvdplm Feb 18, 2021

niklasad1 Feb 18, 2021 •

edited

Loading

niklasad1 Feb 17, 2021

insipx left a comment

dvdplm left a comment

dvdplm Feb 18, 2021

dvdplm Feb 18, 2021

dvdplm Feb 18, 2021

dvdplm Feb 18, 2021

niklasad1 Feb 18, 2021 •

edited

Loading

dvdplm Feb 18, 2021

		@@ -146,43 +159,8 @@ pub enum WsConnectError {

		/// Creates a new WebSocket connection based on [`WsConfig`](crate::WsConfig) represented as a Sender and Receiver pair.
		pub async fn websocket_connection(config: WsConfig<'_>) -> Result<(Sender, Receiver), WsHandshakeError> {

[ws client]: add wss test + refactor WebSocketTransport builder #209

[ws client]: add wss test + refactor WebSocketTransport builder #209

Conversation

niklasad1 commented Feb 12, 2021

Choose a reason for hiding this comment

niklasad1 Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insipx left a comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[ws client]: add `wss` test + refactor `WebSocketTransport builder` #209

[ws client]: add `wss` test + refactor `WebSocketTransport builder` #209

niklasad1 Feb 16, 2021 •

edited

Loading

niklasad1 Feb 18, 2021 •

edited

Loading

niklasad1 Feb 18, 2021 •

edited

Loading