feat(core): replace callbacks in `RpcModule` and `Methods`

[prestwich]

Allows users to remove or replace methods in the Methods and RpcModule collections

closes #1053

this is effectively a successor to #1058, that follows the HashMap idiom 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 not

Also includes insert_or_replace_if and merge_replacing_if as proposed in #1058 pr review

[prestwich]

pushing renamings as you suggest :)

[prestwich]

quick idea:

• Add unsubscribe_method_name as a prop of MethodCallback::Subscription so that we can always identify the unsub method

[prestwich]

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

[niklasad1]

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.

[prestwich]

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 #[doc(hidden)] pub trait HazardousModuleMutations to encapsulate dangerous behavior behind an explicit user opt-in?

[prestwich]

quick idea:

• Add unsubscribe_method_name as a prop of MethodCallback::Subscription so that we can always identify the unsub method

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:

• Subscribe and Unsubscribes now store the name of their corresponding method
• when remove() removes a subscribe or an unsubscribe, it removes the corresponding method as well.
• remove() called on an unsubscribe returns the subscribe
• Aliases are now first-class citizens instead of cloned Arcs. This is necessary to ensure that aliases never allow dangling subscribe/unsubscribe entires

Aliases now work as follows

• MethodCallback can be downgraded to WeakMethodCallback
• the MethodCallback::Alias variant stores a WeakMethodCallback
• the WeakMethodCallback can be upgraded to a MethodCallback, provided the original method still exists (presumably owned by the map)
• method and method_with_name now resolve aliases by checking for MethodCallback::Alias and upgrading the result.
• Aliases with dangling targets are treated like empty map entries for most purposes

However, there are small breaking interface changes

• Changing contents of MethodCallback::Subscription and MethodCallback::Unsubscription
• Adding the Alias variant to the MethodCallback enum
• method and method_with_name now return MethodCallback instead of `&MethodCallback)

Followup questions:

• Should we opportunistically prune dangling aliases? Reaping them would only save a fat pointer and an indirection, so it seems fine to leave them in the map
• should aliases with dangling targets be replacable by insert and register functions? (i.e. should verify_method_name check if the map contains a dangling alias at that method name, and allow replacement if the alias is dangling)

Summary

• Methods cannot contain dangling Subscription or Unsubscription methods
• aliases no longer clone the method Arc, instead they create a weak reference
• dangling aliases are handled and consume almost no memory
• use of the primary API cannot result in invalid map states

[niklasad1]

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....

[prestwich]

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....

So the type of "method callback" must be checked to do this correctly.

Yes exactly. This is the problem the latest commit resolves. Please see how those are handled by the remove function. By ensuring that the Subscription and Unsubscription are removed only as a unit we ensure that there can be no dangling handlers. The danger of using mut_callbacks is called out in the documentation on that function, and all other functions have been rewritten to use remove (see logic of replace, replace_if, merge_replace, etc). This prevents dangling subscriptions entirely by ensuring that an unsubscribe cannot be removed without removing the associated subscription method (and vice-versa)

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 Weak instead of Arc::clone. We now guarantee that for every sub/unsub pair the map always contains a single arc reference to each of sub and unsub, that we add and remove those only as a unit. A sub without an unsub (and vice versa) are treated as invalid map states, and the API prevents them from occurring

[prestwich]

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 replace and replace_if API, or the version in this PR that ensures that the map cannot contain dangling methods?

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?

[jsdw]

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 :)

[prestwich]

cool, any updates on when he'll be back?

[niklasad1]

Hey, I'm back but I'll take a look at this again next week.

Sorry for slow response here

[niklasad1]

Hey again,

I haven't changed my mind regarding the "dangling handler" this because I regard the remove API explicit which applies per RpcModule and states that it's the user's responsibility to remove handlers if it's a subscription but yeah it's a maybe a gray area and perhaps my interpretation is wrong.

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. I'll take some time review this thoroughly, thanks for your patience

[prestwich]

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.

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