rpc: Upgrade WebSocketClient to support full client functionality #646

thanethomson · 2020-10-20T14:02:57Z

In partial fulfillment of #582.

This PR expands the WebSocketClient's capabilities to be able to handle other types of requests other than just subscriptions (i.e. it implements the Client trait).

It also drastically simplifies some of the internal code for the RPC crate.

📖 Rendered updated ADR (008)

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

This commit is multi-faceted. It: 1. Drastically simplifies much of the subscription-related functionality. 2. Expands the WebSocketClient's capabilities to be able to handle other types of requests other than just subscriptions (i.e. it implements the Client trait). Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

codecov-io · 2020-11-10T22:01:16Z

Codecov Report

Merging #646 (281a0a8) into master (b159940) will decrease coverage by 1.1%.
The diff coverage is 55.2%.

@@           Coverage Diff            @@
##           master    #646     +/-   ##
========================================
- Coverage    41.7%   40.6%   -1.2%     
========================================
  Files         191     192      +1     
  Lines       12595   12410    -185     
  Branches     3223    3137     -86     
========================================
- Hits         5262    5042    -220     
- Misses       6991    7048     +57     
+ Partials      342     320     -22

Impacted Files	Coverage Δ
rpc/src/client.rs	`8.5% <ø> (ø)`
rpc/src/client/transport/http.rs	`0.0% <0.0%> (ø)`
rpc/src/endpoint/abci_info.rs	`54.8% <ø> (ø)`
rpc/src/endpoint/abci_query.rs	`22.2% <ø> (ø)`
rpc/src/endpoint/block.rs	`30.7% <ø> (ø)`
rpc/src/endpoint/block_results.rs	`0.0% <ø> (ø)`
rpc/src/endpoint/blockchain.rs	`0.0% <ø> (ø)`
rpc/src/endpoint/broadcast/tx_async.rs	`0.0% <ø> (ø)`
rpc/src/endpoint/broadcast/tx_commit.rs	`0.0% <ø> (ø)`
rpc/src/endpoint/broadcast/tx_sync.rs	`0.0% <ø> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b159940...281a0a8. Read the comment docs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

rpc/src/client.rs

thanethomson · 2020-11-10T23:12:44Z

rpc/src/client/sync.rs

@@ -39,6 +39,7 @@ pub struct ChannelRx<T>(mpsc::UnboundedReceiver<T>);
 impl<T> ChannelRx<T> {
    /// Wait indefinitely until we receive a value from the channel (or the
    /// channel is closed).
+    #[allow(dead_code)]


This method is not used if only the http-client feature is enabled, but putting it behind a feature flag breaks things.

thanethomson · 2020-11-10T23:18:24Z

rpc/src/request.rs

+/// simple, singular response.
+///
+/// [`Subscription`]: struct.Subscription.html
+pub trait SimpleRequest: Request {}


This is the simplest way I could find to configure the type system in such a way that I could restrict access to the Client::perform method to only those kinds of requests that expect a single response (i.e. non-subscription-related requests), while still preserving the existing overall type architecture.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

ChannelTx::send now no longer needs to be mutable because the underlying channel has no mutability requirement. This leads to viral changes throughout the interfaces and clients (positive ones). ChannelTx::send also doesn't need to be async, because it relies on an unbounded channel. This also has some viral implications for other methods throughout the RPC client. If, in future, we want to support bounded channels, then we can consider making it async again. But we probably shouldn't make it async if we don't need it to be. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

greg-szabo · 2020-11-24T14:01:35Z

The latest changes are awesome. When can we review and merge?

melekes

melekes · 2020-11-24T19:12:38Z

rpc/src/client/transport/router.rs

+                );
+            }
+        }
+        // Obtain a mutable reference because the previous reference was


interesting 🤔 i would expect Rust to figure out that the loop is over

for x in y { b } is syntactic sugar for let mut iter = y.into_iter(); while let Some(x) = iter.next() { b } and into_iter() consumes self, therefore one cannot access the collection being iterated over (ie. subs_for_query) after the loop is over.

On the other hand, for x in &y { b } would desugar to a call to iter() which does not not consume self but instead yields reference to the underlying elements. Same with for x in &mut y.

I wonder though if we could use for (id, event_tx) in &subs_for_query here instead?

got it! thanks for explaining it @romac 🙏

So interestingly, the for (id, event_tx) in &subs_for_query didn't work previously, and I've checked again now, it results in the following error:

error[E0277]: `&&mut std::collections::HashMap<std::string::String, client::sync::ChannelTx<std::result::Result<event::Event, error::Error>>>` is not an iterator --> rpc/src/client/transport/router.rs:35:31 | 35 | for (id, event_tx) in &subs_for_query { | ^^^^^^^^^^^^^^^ `&&mut std::collections::HashMap<std::string::String, client::sync::ChannelTx<std::result::Result<event::Event, error::Error>>>` is not an iterator | = help: the trait `std::iter::Iterator` is not implemented for `&&mut std::collections::HashMap<std::string::String, client::sync::ChannelTx<std::result::Result<event::Event, error::Error>>>` = note: required by `std::iter::IntoIterator::into_iter`

for (id, event_tx) in &mut subs_for_query also doesn't compile.

The following code, however, does work, so I'll update the code accordingly:

for (id, event_tx) in subs_for_query.borrow_mut() { // ... }

I feel as though if I manually expanded all the syntactic sugar, I'd understand this better 😁

melekes · 2020-11-24T19:14:21Z

rpc/src/client/transport/router.rs

+                self.subscriptions.get_mut(&query).unwrap()
+            }
+        };
+        subs_for_query.insert(id.to_string(), tx);


what if the id already exists?

It'd be overwritten.

My assumption here is that our UUIDs that we're automatically generating for subscriptions won't ever collide. I suppose that leaves a very small, albeit non-zero, non-deterministic probability of having a subscription dropped arbitrarily, but I'd imagine that networking failures leading to the same outcome would be far higher probability than that of a UUIDv4 collision.

melekes · 2020-11-24T19:17:44Z

rpc/src/client/transport/router.rs

+pub enum PublishResult {
+    Success,
+    NoSubscribers,
+    AllDisconnected,


what is the intuition behind this type of result?

It was one way of passing some additional information back to the WebSocketClient after it attempts to publish an event.

Since the WebSocketClient only sends an unsubscribe request once all subscribers for a particular query have dropped, I needed to be able to distinguish between the case where there were no subscribers to begin with (i.e. we somehow received a spurious event without having a subscription) and the case where we had subscribers, but since we published last to them they dropped their receiving ends. Both would publish the event to zero subscribers.

In the former case, we would consider this to be unexpected behaviour on the part of the node (I don't do anything about it right now though), and in the latter case it'd be a signal to send a single unsubscribe request.

melekes · 2020-11-24T19:39:10Z

rpc/src/client/transport/websocket.rs

-                        }
-                    }
-                },
+    async fn confirm_pending_command(


confirm here means sending response?

Yes. I can rename it to something a bit clearer though.

…wice Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

cargo-dep-bot · 2020-11-25T21:46:21Z

This PR made the following dependency changes:

Added Packages (Duplicate versions in '()'):
	clap 2.33.3
	colored 1.9.3
	heck 0.3.1
	num-derive 0.3.3
	proc-macro-error 1.0.4
	proc-macro-error-attr 1.0.4
	simple_logger 1.11.0
	strsim 0.8.0 (0.9.3)
	structopt 0.3.20
	structopt-derive 0.4.13
	tendermint-rpc-probe 0.17.0-rc3
	textwrap 0.11.0
	unicode-segmentation 1.7.1
	unicode-width 0.1.8
	vec_map 0.8.2

Removed Packages (Remaining versions in '()'):
	abscissa_tokio 0.5.1

Updated Packages:
	async-tungstenite: 0.8.0 -> 0.9.3
	tendermint: 0.16.0 -> 0.17.0-rc3
	tendermint-light-client: 0.16.0 -> 0.17.0-rc3
	tendermint-light-node: 0.16.0 -> 0.17.0-rc3
	tendermint-proto: 0.1.0 -> 0.17.0-rc3
	tendermint-rpc: 0.16.0 -> 0.17.0-rc3
	tendermint-testgen: 0.1.0 -> 0.17.0-rc3

greg-szabo added this to the v0.17.0 milestone Oct 21, 2020

Merge latest changes from master

68b7a82

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson removed this from the v0.17.0 milestone Oct 21, 2020

thanethomson added 4 commits October 21, 2020 12:41

The http-client feature also needs the tracing lib

eef3d7b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Make client mutable in example

b862804

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge latest changes from master

6da9857

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update CHANGELOG

e81759c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added 3 commits November 10, 2020 17:14

Update ADR-008 to reflect recent changes

b938499

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix minor bug in response handling

c384205

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Update tests to use new WebSocketClient functionality

5bf8074

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson marked this pull request as ready for review November 10, 2020 22:51

thanethomson requested review from ebuchman, romac and greg-szabo November 10, 2020 22:53

thanethomson added 3 commits November 10, 2020 18:07

Fix/ignore clippy warnings

8f5fb24

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix documentation for rpc crate

f04b8b4

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix broken link in crate docs

f6eaf88

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson commented Nov 10, 2020

View reviewed changes

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

thanethomson commented Nov 10, 2020

View reviewed changes

thanethomson added 6 commits November 10, 2020 18:22

Update RPC repo docs

cb82342

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge latest changes from master

0658499

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge latest changes from master

bf1565e

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge latest changes from master

c2c3528

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Client no longer needs to be mutable

741cecb

Signed-off-by: Thane Thomson <connect@thanethomson.com>

melekes reviewed Nov 24, 2020

View reviewed changes

thanethomson added 3 commits November 25, 2020 15:42

Use explicit borrow_mut to avoid having to obtain subscriptions map t…

eefced1

…wice Signed-off-by: Thane Thomson <connect@thanethomson.com>

Rename method for clarity

62cbdca

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge latest changes from master

281a0a8

Signed-off-by: Thane Thomson <connect@thanethomson.com>

melekes approved these changes Nov 26, 2020

View reviewed changes

thanethomson merged commit 5105efe into master Nov 26, 2020

thanethomson deleted the rpc/upgrade-websocket-client branch November 26, 2020 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: Upgrade WebSocketClient to support full client functionality #646

rpc: Upgrade WebSocketClient to support full client functionality #646

thanethomson commented Oct 20, 2020 •

edited

Loading

codecov-io commented Nov 10, 2020 •

edited

Loading

thanethomson Nov 10, 2020

thanethomson Nov 10, 2020

greg-szabo commented Nov 24, 2020

melekes left a comment

melekes Nov 24, 2020

romac Nov 24, 2020

melekes Nov 25, 2020

thanethomson Nov 25, 2020

melekes Nov 24, 2020

thanethomson Nov 25, 2020

melekes Nov 24, 2020

thanethomson Nov 25, 2020

melekes Nov 24, 2020

thanethomson Nov 25, 2020

cargo-dep-bot bot commented Nov 25, 2020

rpc: Upgrade WebSocketClient to support full client functionality #646

rpc: Upgrade WebSocketClient to support full client functionality #646

Conversation

thanethomson commented Oct 20, 2020 • edited Loading

codecov-io commented Nov 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-szabo commented Nov 24, 2020

melekes 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

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

cargo-dep-bot bot commented Nov 25, 2020

thanethomson commented Oct 20, 2020 •

edited

Loading

codecov-io commented Nov 10, 2020 •

edited

Loading