Implement subscription support in the client #436
Conversation
|
Can you add a description to guide reviewers perhaps? What triggered the PR and how it goes about improving the status quo, what are the salient points of the PR etc. Thanks! |
|
Sorry, I got assigned other tasks. I'll try to find some time this weekend. |
dvc94ch
force-pushed
the
dvc-client-pubsub
branch
from
90c1433
to
ce8cf50
Compare
dvc94ch
force-pushed
the
dvc-client-pubsub
branch
from
e3fad89
to
d989870
Compare
|
Thanks a lot for this work. I will need this soon! |
Add Deserialize bound for subscription args in client
core-client/src/lib.rs
Outdated
| @@ -5,4 +5,7 @@ | |||
| //! | |||
| //! See documentation of [`jsonrpc-client-transports`](../jsonrpc_client_transports/) for more details. | |||
|
|
|||
| #![deny(missing_docs)] | |||
| #![deny(warnings)] | |||
There was a problem hiding this comment.
We don't deny(warnings) to prevent uncontrolled build failures in newer versions of the compiler or external crates. Please remove.
core-client/transports/src/lib.rs
Outdated
| @@ -1,6 +1,7 @@ | |||
| //! JSON-RPC client implementation. | |||
|
|
|||
| #![deny(missing_docs)] | |||
| #![deny(warnings)] | |||
core-client/transports/src/lib.rs
Outdated
| .into_future() | ||
| .map(move |(result, _)| { | ||
| drop(client); | ||
| result.unwrap() |
There was a problem hiding this comment.
Should we check that the message is one of the three we're sending?
| let request_str = match msg { | ||
| RpcMessage::Call(msg) => { | ||
| let (id, request_str) = self.request_builder.call_request(&msg); | ||
| if self.pending_requests.contains_key(&id) { |
There was a problem hiding this comment.
We should just check the return value of insert instead of doing an extra lookup.
if self.pending_requests.insert(id, msg.sender).is_some() {
log::error!(...);
}
| if map.contains_key(&id) { | ||
| log::error!("reuse of request id {:?}", id); | ||
| } | ||
| map.insert(id.clone(), subscription); |
| RpcMessage::Subscribe(msg) => { | ||
| let (id, request_str) = self.request_builder.subscribe_request( | ||
| msg.subscription.subscribe.clone(), | ||
| msg.subscription.subscribe_params.clone(), |
There was a problem hiding this comment.
Do we need to clone here? Couldn't we destructure the msg.subscription also to avoid repetition?
let SubscribeMessage { subscribe, unsubscribe, notification, subscribe_params, ... } = msg.subscription;
| /// A map from the subscription name to the subscription. | ||
| subscriptions: HashMap<String, HashMap<Id, Subscription>>, | ||
| /// A map from subscription id to id. | ||
| subscription_ids: HashMap<SubscriptionId, Id>, |
There was a problem hiding this comment.
Why do we need so many mappings?
Can't we have:
enum PendingRequest {
Subscription(Subscription),
Call(oneshot::Sender<Result<Value, RpcError>>),
}
pending_requests: HashMap<Id, PendingRequest>,
subscriptions: HashMap<SubscriptionId, Subscription>,
?
The user should guarantee uniqueness of Ids no matter what, we don't need to handle having the same Id for different subscription names.
Address review grumbles
|
Yes, much better |
| /// Incoming messages from the underlying transport. | ||
| stream: TStream, | ||
| /// Unprocessed incoming messages. | ||
| incoming: VecDeque<(Id, Result<Value, RpcError>, Option<String>, Option<SubscriptionId>)>, |
There was a problem hiding this comment.
Could this long tuple be a struct, since also used here: https://github.com/paritytech/jsonrpc/pull/436/files#diff-fea24e9f642ee28ee34be4c83ff3a900R64
* Handle Subscriber wrapped generic type * Pubsub Subscriber wrapper with multiple generic types
The PR closes #412 and #444 . Currently the jsonrpc client ignores #[pubsub ...]. With these changes it should support the pubsub attribute.