feat(core): replace callbacks in RpcModule and Methods
#1387
Conversation
prestwich
changed the title
RpcModule and Methods
|
pushing renamings as you suggest :) |
There was a problem hiding this comment.
Looks clean.
We just need something better for replace_method/remove and similar APIs because it would potentially overwrite a subscription callback with arbitrary method callback and the unsubscription would be still be in the HashMap which isn't good.
Thus, we need to check the kind of the method when it's removed as well to do that correctly.
|
quick idea:
|
|
on reflection, i think that this would have to be a more significant refactor in a separate PR. My proposed solution for now would be to note this possibility in the documentation of every merge/remove/replace/replace_if method |
I can't accept such code but lemme a have look how difficult it's to fix that. |
|
Is the main blocker that it is possible to construct modules with dangling handlers at all? or that users may do that accidentally? If the latter, how would you feel about |
I started on the above and realized that it would be broken by the aliasing system, as you could always alias an unsubscribe method, and leave it dangling regardless of any care in the remove method. This means that we have to track aliases separately. So to that end, I have done the more significant refactor I didn't want to do 🫡 New behavior:
Aliases now work as follows
However, there are small breaking interface changes
Followup questions:
Summary
|
|
It's not only a dangling unsubscribe handler that is an issue but it's possible to i) overwrite a "unsubscribe callback" with "method callback" ii) overwrite a "method callback" with a "subscription without a unsubscribe callback"..... There may other edge-cases as well.... A subscription without a unsubscribe callback is most concerning to me because it may not be possible to unsubscribe then without terminating the connection. So the type of "method callback" must be checked to do this correctly. pub fn merge_replace(&mut self, other: impl Into<Methods>) -> Vec<(&'static str, MethodCallback)> {
let mut other = other.into();
let callbacks = self.mut_callbacks();
let mut removed = Vec::with_capacity(other.callbacks.len());
for (name, callback) in other.mut_callbacks().drain() {
let new = callback.kind();
if let Some(prev) = callbacks.insert(name, callback) {
let old = prev.kind();
match (old, new) {
(MethodKind::Subscription, MethodKind::Subscription)
| (MethodKind::MethodCall, MethodKind::MethodCall)
| (MethodKind::Unsubscription, MethodKind::Unsubscription) => {
removed.push((name, prev));
}
(MethodKind::Subscription, other) if other.is_method_call() => {
todo!("remove unsubscribe handler")
}
(MethodKind::Unsubscription, other) if other.is_method_call() => {
todo!("remove subscription handler")
}
(_other, MethodKind::Subscription) => {
todo!("can't replace method call with subscription handler without unsubscribe handler");
}
_ => {
panic!("Unsupported")
}
};
};
}
removed
}However, I think it's not possible to handle it when a "method call" is replaced by a "subscription call" rather than throwing an error or panic:ing but that defeats the purpose of merge_replace stuff. /cc @jsdw @lexnv Thoughts? I'm not sure about this API anymore and may be better to not support it anyway because it ended up to something way more complicated that I had in mind.... |
Yes exactly. This is the problem the latest commit resolves. Please see how those are handled by the remove function. By ensuring that the The follow-on problem, referenced above, is that the map may contain any number of Arcs of the same subscription function. This is resolved by the new aliasing system that relies on |
|
bumping this :) it looks like removal was added in #1416, allowing dangling methods. Does this mean that this PR is no longer blocked on concerns about dangling methods? I'm happy to rebase or start a new branch to get this across the finish line. Would you prefer a simple For clarification so I can submit better PRs in the future, given your concerns voiced here, were there changes that #1416 made that could have helped this PR get merged? |
| } | ||
|
|
||
| self.remove(name); | ||
| self.replace(name, callback) |
There was a problem hiding this comment.
the remove is redundant, as remove is called in replace
|
Niklas is away for a couple more weeks still, and so just as a heads up @prestwich, it's unlikely that much progress will happen on this PR until he's returned! When back and caught up we'll be able to give better feedback and progress this :) |
|
cool, any updates on when he'll be back? |
|
Hey, I'm back but I'll take a look at this again next week. Sorry for slow response here |
|
Hey again, I haven't changed my mind regarding the "dangling handler" this because I regard the Anyway, this PR will allow merge any number RpcModule's "to be merged" which I believe is a footgun and much harder to know whether something was leaked as we have been discussing earlier. This was not really obvious when making this PR and I think the overall good quality of your PR was good. |
as noted above, in this PR it is impossible to "leak" anything at any point, as the data structure no longer permits dangling handlers at all |
| @@ -521,6 +783,16 @@ impl<Context> From<RpcModule<Context>> for Methods { | |||
| } | |||
|
|
|||
| impl<Context: Send + Sync + 'static> RpcModule<Context> { | |||
| /// Return a mutable reference to the currently registered methods. | |||
| pub fn methods_mut(&mut self) -> &mut Methods { | |||
There was a problem hiding this comment.
We don't want to expose this, can you remove it or make it private?
It's not used currently.
| &mut self, | ||
| alias: &'static str, | ||
| target_name: &'static str, | ||
| ) -> Result<(), RegisterMethodError> { |
There was a problem hiding this comment.
We returned an error if the target_name wasn't not registered before and I would like to keep it that way.
Why did you change that (maybe I'm missing something)?
| /// This type is an implementation detail of the method aliasing system and | ||
| /// should not be used directly. | ||
| #[derive(Clone, Debug)] | ||
| pub enum WeakMethodCallback<'a> { |
There was a problem hiding this comment.
dq: do we really need this Weak stuff if remove/merge actually takes of the subscription handlers?
maybe the alias stuff needs it if it points to a replaced handler.
| /// any). For subscription and unsubscribe methods, the `Subscription` | ||
| /// callback will be returned, regardless of whether the `method_name` | ||
| /// corresponds to the subscription or unsubscription method. | ||
| pub fn remove(&mut self, method_name: &str) -> Option<MethodCallback> { |
There was a problem hiding this comment.
I wonder why not refactor this a bit further to make it more descriptive i.e, that it just returns the "subscribe callback" for removed subscriptions and not the "unsubscribe callback"...
e.g.
enum MethodCall {
Method(Callback),
Subscription { subscribe: Callback, unsubscribe: Callback }
}I don't how useful it's to get the removed callbacks though via this API...
There was a problem hiding this comment.
I don't how useful it's to get the removed callbacks though via this API...
It's not very useful, because they can't be re-registered without low-level access. In order to make it useful, we would have to create a struct from which we can retrieve the underlying handle logic without the wrapping closure created by the register functions. which could get quite messy
It returns on removal because that's the expected API behavior of map types, e.g. HashMap::remove
| subscribe_method_name: &'static str, | ||
| }, | ||
| /// The method is an alias for another method. | ||
| Alias(WeakMethodCallback<'static>), |
There was a problem hiding this comment.
How is this intended to work?
If one enables an alias for method a and method a is replaced by another callback?
Is the alias just disabled i.e, dangling/weak or is supposed to point the new callback?
There was a problem hiding this comment.
If one enables an alias for method a and method a is replaced by another callback?
like the current aliasing system, the alias points to the underlying callback, not to the aliased name (i.e. aliases are not recursively resolved). However, unlike the current aliasing system, the alias no longer keeps the callback "alive" like the canonical name does. Removing the canonical name will remove the callback from the map and the alias's will no longer work
Using a weak is important to preventing dangling handlers. If we allow the alias to keep the callback alive, then we can create dangling handlers by aliasing subscribe functions before removing them
| pub fn method_with_name(&self, method_name: &str) -> Option<(&'static str, MethodCallback)> { | ||
| self.callbacks.get_key_value(method_name).and_then(|(k, v)| { | ||
| let v = match v { | ||
| MethodCallback::Alias(alias) => alias.upgrade()?, |
There was a problem hiding this comment.
Ok, it's just using the inner callback as an alias then.
I think it's better to remove MethodCallback::Alias then it just adds a bunch of confusion when matching on it with unreachable.
There was a problem hiding this comment.
Looks overall reasonable and I left some comments in the code.
In order for me to happy with this I would like to have some tests for this such as such as merge two rpc modules with conflicting subscription and methods works as intended.
Then it looks to me like aliases won't work when after a handler with aliases is replaced, so I want additional info how it's intended to work.
Allows users to remove or replace methods in the
MethodsandRpcModulecollectionscloses #1053
this is effectively a successor to #1058, that follows the
HashMapidiom of returning items when they are replaced. This addresses the earlier PR review that it is possible to replace without realizing it, by making all replacements explicit. The caller can then decide whether to drop the replaced functions or notAlso includes
insert_or_replace_ifandmerge_replacing_ifas proposed in #1058 pr review