[ws client]: kill raw client #171

niklasad1 · 2020-12-04T19:11:10Z

Fixes #154

The major fix is that it splits up the RawClient into a Sender and Receiver part such that in the event loop we don't have destroy the borrow which causes us to drop the future and leave Soketto in a bad state. Because Soketto's async methods are not canceling safe.

As the Sender and Receiver is separate the functionality that keeps track of the state of pending requests, pending subscription and active subscriptions are now handled by a new type RequestManager, that lives in background task.

The rationale behind this change is that the `insert_methods` takes ownership of the `send_back_oneshot` and if the operation fails it should be propagated the frontend. So returning the `Err(send_back_oneshot)` if it fails makes it possible.

src/client/ws/jsonrpc_transport.rs

niklasad1 · 2020-12-10T21:51:06Z

src/client/ws/manager.rs

+		request_id: RequestId,
+		subscription_id: SubscriptionId,
+	) -> Option<(SubscriptionSink, UnsubscribeMethod)> {
+		match (self.requests.entry(request_id), self.subscriptions.entry(subscription_id)) {


Entry API takes ownership of the key, we could do:

let request = matches!(self.request.get(&request_id), Some(Kind::Subscription(_)); let sub = subscriptions.contains_key(&subscription_id); if request && sub { let r = self.requests.remove(request_id); let _ = self.subscriptions.remove(subscription_id); match r { Kind::Subscription(send_back) => Some(send_back), _ => unreachable!(), } } else { None }

Still have to do two lookups though :(

//cc @dvdplm thoughts?

niklasad1 · 2020-12-10T21:52:31Z

src/client/ws/raw.rs

@@ -1,710 +0,0 @@
-// Copyright 2019 Parity Technologies (UK) Ltd.


Removed RawClient

niklasad1 · 2020-12-10T21:53:23Z

src/client/ws/transport.rs

@@ -275,7 +261,7 @@ impl<'a> WsTransportClientBuilder<'a> {
 		};

 		// Configure a WebSockets client on top.
-		let mut client = WsRawClient::new(tcp_stream, &self.host, &self.url);
+		let mut client = WsRawClient::new(BufReader::new(BufWriter::new(tcp_stream)), &self.host, &self.url);


optimization

niklasad1 · 2020-12-10T21:55:38Z

src/ws/raw/tests.rs

@@ -1,94 +0,0 @@
-// Copyright 2019 Parity Technologies (UK) Ltd.


Removed these tests because we are testing it via WsServer tests anyway

tests/integration_tests.rs

…ee into na-v2-kill-raw-client

src/client/ws/transport.rs

src/client/ws/client.rs

src/client/ws/manager.rs

Co-authored-by: David <[email protected]>

src/client/ws/client.rs

It was hard to use when the expected id is not known.

niklasad1 · 2020-12-16T16:59:16Z

src/client/ws/client.rs

+					match process_response(&mut manager, response, config.subscription_channel_capacity) {
+						Ok(Some((unsubscribe, params))) => {
+							if let Err(e) = sender.start_request(unsubscribe, params).await {
+								log::error!("Failed to send unsubscription response: {:?}", e);


Note, we could terminate the client here also because the server was probably shut down if the request failed.

niklasad1 · 2020-12-16T16:59:34Z

src/client/ws/client.rs

-							sender.start_request(unsubscribe, params).await.unwrap();
+						match process_response(&mut manager, response, config.subscription_channel_capacity) {
+							Ok(Some((unsubscribe, params))) => {
+								if let Err(e) = sender.start_request(unsubscribe, params).await {


Note, we could terminate the client here also because the server was probably shut down if the request failed.

niklasad1 · 2020-12-16T17:16:58Z

@dvdplm I think this is ready for review

src/client/ws/manager.rs

src/client/ws/jsonrpc_transport.rs

dvdplm · 2020-12-17T10:48:46Z

src/client/ws/transport.rs

@@ -275,7 +261,7 @@ impl<'a> WsTransportClientBuilder<'a> {
 		};

 		// Configure a WebSockets client on top.
-		let mut client = WsRawClient::new(tcp_stream, &self.host, &self.url);
+		let mut client = WsRawClient::new(BufReader::new(BufWriter::new(tcp_stream)), &self.host, &self.url);


dvdplm · 2020-12-17T10:50:27Z

src/client/ws/transport.rs

@@ -254,7 +241,7 @@ impl<'a> WsTransportClientBuilder<'a> {
 	}


with_url, with_origin_header and with_timeout are not used it seems, can delete?

I prefer the keep those if we decide to make the client generic or expose more builder options but I could export those to "silence" the warnings.

I see. Or just add #[allow(unused)].

Fixed in 1f904e1

src/types/error.rs

* Add better documentation * Rename methods. * Add `proof` to unreachable!

src/client/ws/manager.rs

niklasad1 · 2020-12-17T19:45:21Z

src/client/ws/client.rs

+					// NOTE: The subscription may have been closed earlier if
+					// the channel was full or disconnected.
+					if let Some(request_id) = manager.get_request_id_by_subscription_id(&sub_id) {
+						if let Some((_sink, unsubscribe_method)) = manager.remove_subscription(request_id, sub_id.clone()) {


NOTE: I missed to send a request to the unsubscribe method before.

I should write a test for this but..... not in this PR :)

niklasad1 · 2020-12-17T19:46:12Z

src/client/ws/client.rs

+					// the channel was full or disconnected.
+					if let Some(request_id) = manager.get_request_id_by_subscription_id(&sub_id) {
+						if let Some((_sink, unsubscribe_method)) = manager.remove_subscription(request_id, sub_id.clone()) {
+							if let Ok(json_sub_id) = jsonrpc::to_value(sub_id) {


this should be infallible but....

src/client/ws/manager.rs

* update http example * ungeneric crate * update dependencies * [client]: add WebSocket client again. * [deps]: remove needless dev dependencies * nits: forgot to commit new files * [ws client]: `send_text` instead of `send_binary` This is temporary fix to work with the `server` which assumes that `WebSocket` resonses are `text` * chore: add a bunch of more logging * [ws]: port tests but some are failing. * chore: fmt * [server API]: expose `fn local_addr` as public API. * [tests]: make them less ugly by using `127.0.0.1:0` * fix: a bunch of compiler warnings. * [api]: uniform naming, `bind -> new` in transport. * [websocket server]: reply when deserial fails When the server receives an request with invalid JSON `-32700, Parse error shall be returned` * chore: fmt * [tests]: fix remaining tests * [websocket server]: support `binary` and `text` * [ws server]: fix bug in subscription response. `.await` was missing in RegisteredSubscription::send() and no responses were actually sent which this commit fixes. * [client API]: export `WsSubscription` * [examples]: use `localhost` instead of `127.0.0.1` Hostname is required when using `wss` and `127.0.0.1` is not valid hostname. * [examples]: add subscription example. * chore: fmt * [ws server]: fix bug register new subscription. Fixes newly introduced bug that causes `register_subscription` to have side-effects even if the subscription fails. * fmt * more uniform logs * [ws server]: simple subscription test. * [ws server]: subscription tests improved. * [tests]: extract test helpers to a separate crate (#125) * [ws server]: don't close connection when `deserialization` fails (#131) * [ws server]: don't close conn. when `deser` fails * Update src/ws/transport.rs * grumbles: prefer matching of if else. * chore: CI warn `intra_doc_link_resolution_failure` (#139) Since we have not updated the documentation properly it's annoying that the entire job fails. * chore: rustfmt.toml (#138) * chore: add `rustfmt.toml` for formatting * style: `cargo fmt --all` with new config * [server raw params]: fix debug implementation (#137) * [server]: simply raw params impl Use debug implementation of `common::Params` instead of doing something similar that doesn't work properly. * [raw params]: derive `Debug` impl. * [ws server]: parse subscription ID for unsubscription instead of hardcoding `JsonValue::Null` (#136) * [ws server]: fix broken unsubscribe. Try to parse the subscription ID as the first element of an Array or the `subscription` field of an Object/Map. If both of those fails then regard it as a error. * fmt * fix grumbles: remove space indentation * fix(ws server): sub/unsubscribe to same method should generate an error (#140) * fix(ws server): sub/unsubscribe to same method err Subscribe and unsubscribe to the same method should generate an error, which this commit fixed. This bug was introduced by myself in fc87889 * Update src/ws/server.rs Co-authored-by: David <[email protected]> Co-authored-by: David <[email protected]> * chore: add naive benches for request/response (#142) Co-authored-by: Niklas <[email protected]> * fix(ws server): remove faulty debug_assert (#145) The code assumed that `subscription id` is still in `active_subscriptions` when the connection was dropped. The list of subscriptions (kept in raw server) are not notified when a client dropped its subscription/unsubscribed thus it's possible that the actual subscriptions are closed before the entire client was dropped. * ci(benches): cargo check on benches. (#146) * fix(http client): implement `clone` uniform API. (#147) * chore(deps): update `futures v0.3.7` (#148) * chore(deps): update remaining crates (#149) * chore(deps): update `futures v0.3.7` * chore(deps): bump the rest of deps * Improve HTTP client background thread (#150) * refactor: resultify API + some crate reorg (#144) * [ws client]: resultify API and fix subscribe. * The commit changes the API to return `Err` when it's possible and to not ignore underlying errors. * Fix that `fn subscribe` doesn't accept the subscription and unsubscription to be same which causes errors in the server. * nits: Err::SubscriptionMetod -> Err::Subscription * refactor(client): common error type * refactor(http client): resultify * refactor(common): rename common -> types.. This commit renames the `common module` to `types` and tries to distinguish the types that is directly related to the `JSON-RPC v2 specification` from others. Somethings are a little big sloppy named as naming is hard. Also, as bonus a removed a bunch of needless stuff in http server related to subscription. * Update src/ws/tests.rs * style: cargo fmt * fix(grumble): matches -> assert(matches) * fix(grumbles): `jsonrpc_v2` -> `jsonrpc` * fix(nit): remove unused code. * fix(benches): make it compile again. * style: cargo fmt * fix nits (#151) * fix(ws client): send binary (1 byte less payload) * docs(ws server): fix bad comment. * chore: add `editorconfig` (#152) * chore: make `debug log` less verbose. (#153) * chore: make `debug log` less verbose. The debug logging was just too verbose and this commit simplies it as follows: ``` DEBUG recv: {"jsonrpc":"2.0","method":"<METHOD>","params":<PARAMS>,"id":<ID>} DEBUG send: {"jsonrpc":"2.0","result":"<RESULT>","id":<ID>} ``` * style: cargo fmt * fix: missed logs * [jsonrpc types]: implement Display for Request/Response (#160) * feat(jsonrpc response/request): impl `Display` * refactor(logging): use display impl * use serde_json for verbosity * [http client]: refactor with "syncronous-like" design (#156) * experimental * ci(benches): sync and concurrent roundtrips Improve benchmarks to take concurrent requests into account. * ci(benches): sync and concurrent roundtrips Improve benchmarks to take concurrent requests into account. * fix(nits) * feat(http client): limit max request body size * test(http transport): request limit test * test(http client): add tests. * fix typo * fix(benches): make it compile again. * fix(ws example): revert unintentional change. * test(http client): subscription response on call. * fix(cleanup) * fix(benches): make it compile again. * Update src/client/http/transport.rs * fix(http client): `&str` -> `AsRef<str>` * docs(client types): better docs for Mismatch type. * style: `Default::default` -> `HttpConfig::default` * fix(http client): read body size from header. Expermential to read number of bytes from `HTTP Content Length` to pre-allocate the number of bytes and bail early if the length is bigger than the `max_request_body size` Need to be benched with bigger requests. * test(raw http): enable tests to works again. * style: cargo fmt * benches: address grumbles * feat(jsonrpc response/request): impl `Display` * refactor(logging): use display impl * fix(http client): nits. * Update benches/benches.rs * fix bad merge. * chore(deps): update dependencies. (#164) * feat(http server): configurable request body limit (#162) * feat(http server): configurable request body limit * refactor(crate reorg): to have shared http helpers. * Merge client and server errors. * Move `http_server_utils` to `utils/http` * Minor cleanup * fix nits * fix(hyper helper): u64 -> u32 * Update src/utils/http/hyper_helpers.rs Co-authored-by: David <[email protected]> * Update src/utils/http/hyper_helpers.rs Co-authored-by: David <[email protected]> * fix: grumbles * Update src/utils/http/hyper_helpers.rs Co-authored-by: David <[email protected]> * Update src/http/server.rs Co-authored-by: David <[email protected]> Co-authored-by: David <[email protected]> * ci: remove nightly (#167) Use stabilized `broken_intra_doc_links` instead of `intra_doc_link_resolution_failure` * fix(websocket client): drop subscriptions that can't keep up with the internal buffer size (#166) * fix(ws client): drop subscriptions when full. This commit changes the behavior in the `WebSocket Client` where each subscription channel is used in a non-blocking matter until it is determined as full or disconnected. When that occurs the channel is simply dropped and when the user `poll` the subscription it will return all sent subscriptions before it was and terminate (return None) once it's polled one last time. Similarly as `Streams` works in Rust. It also adds configuration for the `WebSocket Client` to configure capacity for the different internal channels to avoid filling the buffers when it's not expected. * tests(ws client): simple subscription test. * fix: nits * Update src/client/ws/client.rs * refactor(tests): introduce integration_tests Make the repo structure more understable w.r.t testing. * chore(license): add missing license headers * Update src/client/ws/client.rs * Update src/client/ws/client.rs * style: remove unintended spaces. * tests: add concurrent deadlock test Ensure that if more than the requested channel buffer capacity is exceeded it should not deadlock. Such as spawning alot of concurrent requests, notifications or new subscriptions. * Update src/client/ws/client.rs * fix: review grumbles * fix nits: `remove needless closure` * fix: cargo fmt * Update src/client/ws/client.rs Co-authored-by: David <[email protected]> * fix more nits Co-authored-by: David <[email protected]> * fix(ws client): embed request id in `SubscriptionClosed` (#170) * fix(ws client): embed request id SubscriptClosed Fixes #169 * Update src/client/ws/client.rs * Update src/client/ws/client.rs Co-authored-by: David <[email protected]> * Update src/client/ws/client.rs Co-authored-by: David <[email protected]> Co-authored-by: David <[email protected]> * chore(deps): bump dependencies (#172) * [ws client]: add tests (#134) * [test utils]: add `internal_err` and consts [errors]: unify client/server errors [test utils]: fake WebSocket jsonrpc server [ws client]: export errors [ws client]: add some basic tests * fmt * remove log target * fix nits * [ws client]: add subscription test * revert unintendend changes. * fmt * [ws client]: fix panic in tests * cleanup * tests(ws client): test for invalid request ID. * fix nits * [ws client]: kill raw client (#171) * getting started * WIP WIP * cleanup * cleanup v2 * cleanup v3 * perf: use BufReader BufWriter * fix(request manager): resultify insert API The rationale behind this change is that the `insert_methods` takes ownership of the `send_back_oneshot` and if the operation fails it should be propagated the frontend. So returning the `Err(send_back_oneshot)` if it fails makes it possible. * fix nits * examples(ws): revert changes * Update tests/integration_tests.rs * nits: fix unwraps * Update src/client/ws/manager.rs Co-authored-by: David <[email protected]> * Update src/client/ws/transport.rs Co-authored-by: David <[email protected]> * Update src/client/ws/client.rs Co-authored-by: David <[email protected]> * fix build * refactor: simplify `Error::InvalidRequestId` It was hard to use when the expected id is not known. * fix(ws client): error handling. * fix(grumble error type): better error message. * fix(grumble): docs `JSONRPC WebSocket transport` * fix(ws manager): fix grumbles. * Add better documentation * Rename methods. * Add `proof` to unreachable! * fix(ws manager): fix nit in docs. * fix(grumbles): ws client * fix more nits * fix compile warning: export websocket transports. * Update src/client/ws/manager.rs Co-authored-by: David <[email protected]> * deps: tokio 1.0 and hyper 0.14 (#176) * deps: tokio 1.0 and hyper 0.14 * Update Cargo.toml * refactor: crate re-organization with separate crates (#177) * [ci]: feature `http` and `ws` removed. * refactor: re-org crate with smaller crates. * fmt * [ci]: remove default features Currently there are no features in the crates, so that check is not needed. * [http client]: remove unused dependency tokio * docs(http client): fix nits tokio 0.2 -> tokio 1.0 (#178) * docs(http client): tokio 0.2 -> tokio 1.0 * fix: better link * [ci]: github actions (#179) * docs(http client): tokio 0.2 -> tokio 1.0 * [ci]: remove travis * [ci]: add github actions. * [ci]: fix identation nits * [ci]: use cache for actions * [ci]: filter to clippy * [ci]: remove hacks * separate action for ci and benchmarks * [ci]: tweak to run on master branch. * examples/subscription -> examples/ws_subscription * force CI Co-authored-by: David <[email protected]> Co-authored-by: Maciej Hirsz <[email protected]> Co-authored-by: Atkins <[email protected]>

niklasad1 added 10 commits December 4, 2020 13:56

getting started

3ae59a0

Merge remote-tracking branch 'origin/v2' into na-v2-kill-raw-client

196d138

WIP WIP

b7519ce

cleanup

ec46909

cleanup v2

4515a92

cleanup v3

8b8aa93

perf: use BufReader BufWriter

b0e5ab0

fix(request manager): resultify insert API

fc680d2

The rationale behind this change is that the `insert_methods` takes ownership of the `send_back_oneshot` and if the operation fails it should be propagated the frontend. So returning the `Err(send_back_oneshot)` if it fails makes it possible.

fix nits

c9a598c

examples(ws): revert changes

93de4d8

niklasad1 commented Dec 10, 2020

View reviewed changes

src/client/ws/jsonrpc_transport.rs Outdated Show resolved Hide resolved

niklasad1 commented Dec 10, 2020

View reviewed changes

src/client/ws/raw.rs

@@ -1,710 +0,0 @@

// Copyright 2019 Parity Technologies (UK) Ltd.

Copy link

Member Author

niklasad1 Dec 10, 2020

Choose a reason for hiding this comment

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

Removed RawClient

niklasad1 commented Dec 10, 2020

View reviewed changes

tests/integration_tests.rs Outdated Show resolved Hide resolved

Update tests/integration_tests.rs

2ebfde0

niklasad1 marked this pull request as ready for review December 10, 2020 21:58

niklasad1 requested review from dvdplm and maciejhirsz December 10, 2020 21:58

niklasad1 added 2 commits December 11, 2020 10:17

nits: fix unwraps

c2f3248

Merge branch 'na-v2-kill-raw-client' of github.com:paritytech/jsonrps…

dcba2ba

…ee into na-v2-kill-raw-client

niklasad1 changed the title ~~[ws client]: WIP kill raw client~~ [ws client]: kill raw client Dec 11, 2020

dvdplm reviewed Dec 11, 2020

View reviewed changes

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

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

src/client/ws/manager.rs Outdated Show resolved Hide resolved

src/client/ws/manager.rs Outdated Show resolved Hide resolved

niklasad1 and others added 5 commits December 11, 2020 15:11

Update src/client/ws/manager.rs

ab324a3

Co-authored-by: David <[email protected]>

Update src/client/ws/transport.rs

933b7a1

Co-authored-by: David <[email protected]>

Update src/client/ws/client.rs

c69a414

Co-authored-by: David <[email protected]>

fix build

d3f2e10

Merge remote-tracking branch 'origin/v2' into na-v2-kill-raw-client

bb8e4a4

niklasad1 commented Dec 15, 2020

View reviewed changes

src/client/ws/client.rs Show resolved Hide resolved

niklasad1 added 2 commits December 16, 2020 17:20

refactor: simplify Error::InvalidRequestId

bb5127e

It was hard to use when the expected id is not known.

fix(ws client): error handling.

d664bd7

niklasad1 commented Dec 16, 2020

View reviewed changes

dvdplm reviewed Dec 17, 2020

View reviewed changes

niklasad1 added 7 commits December 17, 2020 16:48

fix(grumble error type): better error message.

caed258

fix(grumble): docs JSONRPC WebSocket transport

b911b78

fix(ws manager): fix grumbles.

f735428

* Add better documentation * Rename methods. * Add `proof` to unreachable!

fix(ws manager): fix nit in docs.

00ce842

fix(grumbles): ws client

477b0a8

fix more nits

f6dca29

fix compile warning: export websocket transports.

1f904e1

dvdplm reviewed Dec 17, 2020

View reviewed changes

src/client/ws/manager.rs Outdated Show resolved Hide resolved

niklasad1 commented Dec 17, 2020

View reviewed changes

src/client/ws/manager.rs Outdated Show resolved Hide resolved

Update src/client/ws/manager.rs

b3c2e97

dvdplm merged commit 0daba9d into v2 Dec 17, 2020

dvdplm deleted the na-v2-kill-raw-client branch December 17, 2020 21:13

niklasad1 mentioned this pull request Dec 21, 2020

WASM friendly dependencies #175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ws client]: kill raw client #171

[ws client]: kill raw client #171

niklasad1 commented Dec 4, 2020 •

edited

Loading

niklasad1 Dec 10, 2020 •

edited

Loading

niklasad1 Dec 16, 2020

niklasad1 Dec 10, 2020

niklasad1 Dec 10, 2020

dvdplm Dec 17, 2020

niklasad1 Dec 10, 2020

niklasad1 Dec 16, 2020

niklasad1 Dec 16, 2020

niklasad1 commented Dec 16, 2020

dvdplm Dec 17, 2020

dvdplm Dec 17, 2020

niklasad1 Dec 17, 2020

dvdplm Dec 17, 2020

niklasad1 Dec 17, 2020

niklasad1 Dec 17, 2020

niklasad1 Dec 17, 2020

		@@ -1,710 +0,0 @@
		// Copyright 2019 Parity Technologies (UK) Ltd.

		@@ -1,94 +0,0 @@
		// Copyright 2019 Parity Technologies (UK) Ltd.

		@@ -254,7 +241,7 @@ impl<'a> WsTransportClientBuilder<'a> {
		}

[ws client]: kill raw client #171

[ws client]: kill raw client #171

Conversation

niklasad1 commented Dec 4, 2020 • edited Loading

niklasad1 Dec 10, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Dec 16, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Dec 4, 2020 •

edited

Loading

niklasad1 Dec 10, 2020 •

edited

Loading