Generic PubSub implementation #5456
Conversation
tomusdrw
added
A0-pleasereview 🤓
| #[pubsub(name = "parity_subscription")] { | ||
| /// Subscribe to changes of any RPC method in Parity. | ||
| #[rpc(name = "parity_subscribe")] | ||
| fn parity_subscribe(&self, Self::Metadata, Subscriber<Value>, String, Params); |
There was a problem hiding this comment.
Shouldn't SubscriptionId be returned somewhere?
There was a problem hiding this comment.
The way it works is through Subscriber::assign_id method. Two reasons for that:
- You can do it asynchronously (that's a shitty argument, cause it could just return
Future) - Only after you assign ID you can start sending notification and it's enforced by the signature:
fn assign_id(self, id: SubscriptionId) -> Sink<T>
Dropping Subscriber rejects the subscription as does Subscriber::reject(self, Reason) method.
rpc/src/v1/metadata.rs
Outdated
| return false; | ||
| } | ||
|
|
||
| true |
There was a problem hiding this comment.
self.session's value doesn't have to be equal?
There was a problem hiding this comment.
Actually ParitalEq implementation is not needed at all. Thanks for spotting!
rpc/src/v1/metadata.rs
Outdated
| } | ||
|
|
||
| // impl cmp::PartialEq for Metadata { | ||
| // fn eq(&self, other: &Metadata) -> bool { |
There was a problem hiding this comment.
if it can be removed, better to remove it outright rather than comment it out
| let pm2 = poll_manager.clone(); | ||
|
|
||
| let timer = tokio_timer::wheel() | ||
| .tick_duration(Duration::from_millis(500)) |
There was a problem hiding this comment.
Any reason to set the tick duration to 500ms when the only timer spawned has a granularity of 1 second?
There was a problem hiding this comment.
In worst case it would cause 1 second delay before first poll is triggered, I thought that 500ms is a bit better tradeoff.
| meta.session = None; | ||
|
|
||
| let mut poll_manager = self.poll_manager.write(); | ||
| let (id, receiver) = poll_manager.subscribe(meta, method, params); |
There was a problem hiding this comment.
Any protection against subscribing to parity_subscribe?
There was a problem hiding this comment.
rphmeier
added
A8-looksgood 🦄
|
I have a couple questions about how we can improve pub-sub in future PRs:
|
|
I've made a list of all RPC methods we support, and groupped them by possible cause of change: We can later on optimize polling if following events are exposed by Parity:
This will cover most of the cases, for the rest we can still rely on polling, maybe polling time could be configured depending on the methods, but I don't really want to expose polling timeout to RPC user. To implement proper poll-on-event system we can start with a list of string method names, and later maybe allow some metadata in RPC method definition (like: |
This allows to subscribe to any RPC method. Parity does the polling internally (every 1second).
In future we can optimize it, most of the cases can be polled for instance when new block arrives.
@ngotchac Maybe UI can switch for that, might be much better especially for hosted environments.