IDisposable interface ambiguity on receiver side when marshaling with RpcMarshalableAttribute

[Jemmy1228]

Summary

This is an issue related to the RpcMarshalableAttribute which is proposed in #774 and merged in #777
According to my understanding, the IDisposable interface is ambiguous on the receiver side when marshaling with RpcMarshalableAttribute.

Example

Let's think of a scenario that the ITest interface should be marshaled instead of serialized.

[RpcMarshalable]
public interface ITest : IDisposable
{
    /* Omitted */
}

The RPC server (sender):

public interface IServer
{
    Task<ITest> GetTest();
}

And the RPC client (receiver) uses it as:

ITest test = await jsonRpc.GetTest();
/* Do something with the ITest */
test.Dispose();

Problem

RpcMarshalableAttribute required the marshaled interface to derive from IDisposable interface (to dispose the proxy?).
It seems to me that, the Dispose() method called on the receiver side would cause two things to happen:

1. Disposal of the generated proxy on the receiver side, causing JsonRpc on both sides mark the RPC handle as disposed, and the sender side removes the corresponding Object out of context.
2. Calling IDisposable.Dispose() on the original object on the sender side.

So, calling Dispose() method on the receiver side is rather ambiguous.
Sometimes we only want to dispose the proxy on receiver side without disposing the original object on sender side. However, I don't know how to achieve this.

Probable Solution?

How about defining a dedicated interface for marshaling like this:

public interface IRpcMarshalable
{
    /* This method should be overridden by the Proxy generator on receiver side */
    void DisposeProxy()
    {
        throw new InvalidOperationException("Disposal of proxy should only be done on the receiver side.");
    }
}

And require any interface to be marshaled derive from this interface:

public interface ITest : IRpcMarshalable
{
    /* Omitted */
}

The IRpcMarshalable interface has a default implementation of DisposeProxy(), which won't be overridden on the sender side.
The receiver side proxy generate should override DisposeProxy() method to dispose the proxy and handle in JsonRpc context.

With this approach, IDisposable won't be a required interface to derive from, and the ambiguity disappears.

Remind receiver to dispose proxies

In #774 (comment) concerned about to remind the receiver to dispose the proxies

I think we should start with requiring any additional interfaces to derive from IDisposable, since that makes it more obvious to the receiver that they should dispose of these proxies. If we need to we can always remove that requirement, but adding it later would be a breaking change, so I prefer to start conservatively.

Maybe we can write an Roslyn Code Analyzer to warn that these proxies should be disposed?

If this approach is acceptable, I would be happy to implement it and create a PR.

[AArnott]

Thanks for the excellent explanation and for proposing a fix.

Sometimes we only want to dispose the proxy on receiver side without disposing the original object on sender side. However, I don't know how to achieve this.

This is not supported today. You should consider that any object marshaled across RPC is now 'owned' by the receiver.
.NET itself makes ownership ambiguous, I'm afraid. If RPC wasn't involved and you passed a disposable object around, it's not always clear who should dispose it (enough so that some .NET built-in classes take the reference and a bool indicating whether they should assume ownership). But when RPC is introduced, memory leaks become part of the concern as well. If the sender of the object were to dispose of their object, StreamJsonRpc cannot know about that because disposal doesn't raise any kind of event. That means the only sure way to avoid a memory leak is to watch for the receiver to dispose of the object. Since we create the proxy, we get a chance to notice when Dispose is called on it.
So in short, we saw an existing ambiguity and made a design decision that would solve an RPC-unique problem, without straying beyond the bounds of the original ambiguity.

And require any interface to be marshaled derive from this interface

Your proposal would be a breaking change at this point.

You've obviously given this a lot of thought. Thank you. I'd be interested in continuing to brainstorm on a solution to this with you.

What if we take a variant of your idea: Allow for any marshaled object to derive from either IDisposable or IRpcDisposable. The RPC interface might not itself derive from IRpcDisposable because the point is only the sender would have its implementation. Essentially, if an objects should not pass its ownership to the receiver, it derives from the new interface. This new interface would include an event that StreamJsonRpc can subscribe to for notification when its disposal method is called, and that's when resources are released (on both sides.)
That doesn't give the receiver any control over when resources are released (other than breaking the JSON-RPC connection), but then, in our current design the sender has no control. So it's just providing a way for the application to decide whether ownership transfers or not.

Thoughts?