microsoft / microsoft/vs-streamjsonrpc

IDisposable interface ambiguity on receiver side when marshaling with RpcMarshalableAttribute

Open
#931 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
937
Forks
178
Avg merge
3d 1h
Merged PRs (30d)
28

Description

Summary

This is an issue related to the RpcMarshalableAttribute which is proposed in https://github.com/microsoft/vs-streamjsonrpc/issues/774 and merged in https://github.com/microsoft/vs-streamjsonrpc/pull/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 https://github.com/microsoft/vs-streamjsonrpc/issues/774#issuecomment-1058202291 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with doc/rpc_marshalable_objects.md and the discussion in issue 774 and pull request 777 to understand the existing marshaling and disposal contract. The work is done when the receiver-versus-sender disposal behavior is agreed and the resulting API or analyzer scope is clearly defined.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
api, backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.