AddRemoteRpcTarget doesn't work with methods returning IAsyncEnumerable

[matteo-prosperi]

When using AddRemoteRpcTarget to forward messages to another JsonRpc instance, calling methods that return an IAsyncEnumerable fails.
This is because JsonRpc.rpcTargetInfo.TryGetTargetMethod will always return a match for the special methods that are used to implement the async enumerable support.

This can easily be reproduced with the following code:

using Nerdbank.Streams;
using StreamJsonRpc;
using StreamJsonRpc.Protocol;
using System.Diagnostics;

var (clientStream, manInTheMiddleClientStream) = FullDuplexStream.CreatePair();
var (manInTheMiddleServerStream, serverStream) = FullDuplexStream.CreatePair();

JsonRpc clientRpc;

// client
{
    var clientFormatter = new JsonMessageFormatter();
    var clientHandler = new LengthHeaderMessageHandler(clientStream.UsePipe(), clientFormatter);
    clientRpc = new JsonRpc(clientHandler);
    clientRpc.TraceSource = new TraceSource("Client", SourceLevels.Verbose);
    clientRpc.StartListening();
}

// man in the middle
{
    var manInTheMiddleClientFormatter = new JsonMessageFormatter();
    var manInTheMiddleClientHandler = new LengthHeaderMessageHandler(manInTheMiddleClientStream.UsePipe(), manInTheMiddleClientFormatter);
    var manInTheMiddleClientRpc = new JsonRpc(manInTheMiddleClientHandler);
    manInTheMiddleClientRpc.TraceSource = new TraceSource("ManInTheMiddleClient", SourceLevels.Verbose);

    var manInTheMiddleServerFormatter = new JsonMessageFormatter();
    var manInTheMiddleServerHandler = new LengthHeaderMessageHandler(manInTheMiddleServerStream.UsePipe(), manInTheMiddleServerFormatter);
    var manInTheMiddleServerRpc = new JsonRpc(manInTheMiddleServerHandler);
    manInTheMiddleServerRpc.TraceSource = new TraceSource("ManInTheMiddlServer", SourceLevels.Verbose);

    manInTheMiddleClientRpc.AddRemoteRpcTarget(manInTheMiddleServerRpc);
    manInTheMiddleServerRpc.AddRemoteRpcTarget(manInTheMiddleClientRpc);

    manInTheMiddleClientRpc.StartListening();
    manInTheMiddleServerRpc.StartListening();
}

// server
{
    var serverFormatter = new JsonMessageFormatter();
    var serverHandler = new LengthHeaderMessageHandler(serverStream.UsePipe(), serverFormatter);
    var mserverRpc = new JsonRpc(serverHandler, new Server());
    mserverRpc.TraceSource = new TraceSource("Server", SourceLevels.Verbose);
    mserverRpc.StartListening();
}

// This one works
var result = await clientRpc.InvokeAsync<int>("DoSomethingAsync", 5);

// This doesn't work because JsonRpc.rpcTargetInfo.TryGetTargetMethod has a few hardcoded methods that are never passed to remote targets.
await foreach (var number in await clientRpc.InvokeAsync<IAsyncEnumerable<int>>("EnumerateAsync", 5))
{
    Console.WriteLine(number);
}

Console.ReadKey();

public interface IServer
{
    Task<int> DoSomethingAsync(int val);

    IAsyncEnumerable<int> EnumerateAsync(int val);
}

public class Server : IServer
{
    public Task<int> DoSomethingAsync(int val)
    {
        return Task.FromResult(val);
    }

    public async IAsyncEnumerable<int> EnumerateAsync(int val)
    {
        for (int i = 0; i < val; i++)
        {
            await Task.Yield();
            yield return i;
        }
    }
}

[AArnott]

The IAsyncEnumerator<T> support involves a custom protocol with $/enumerator/next and $/enumerator/abort methods that are not implemented by any target object. For this to work with remote target objects, considering that enumerators may exist on any local target object as well as any remote target object, and considering the remote targets may generate overlapping object IDs with those generated locally, we would have a serious ambiguity problem for an incoming $/enumerator/* method call to know who to dispatch to.
The request ID already would have this ambiguity problem, except that's a core json-rpc protocol invariant and we use an ID map to assign a unique ID that we can dispatch with later.
If the local JsonRpc instance understood that an IAsyncEnumerable<T> was in use, it could create a map of remote IDs to locally generated ones so that no ID would overlap and an incoming ID could be dispatched based on that map. But for messages sent to a remote target, we have no idea what the types involved in the request args or result value are, so we don't know where an enumerator is being used in order to change/map IDs used within it.
I'm not sure how or if we can fix this.

[matteo-prosperi]

I cannot think of an option that would work fully with a mix of local and remote targets and overlapping IDs.
Do you think it would be a good improvement to make it work at least when there are no locally registered async enumerables? We could allow calls to go to a remote target if the ID for the async enumerable is not found. IProgress<T> already works that way.
Personally, I am not interested in mixing local and remote targets, I am more interested in having remote targets only but intercepting and editing messages (or possibly dropping/injecting messages).

Another option would be to allow (when multiple targets are supposed to be supported) to add a prefix to ids of async enumerables, IProgress<T> and marshalable interfaces. Changing "method": "$/enumerator/next", "params": [1] into "method": "$/enumerator/next", "params": ["middleman/1"].

[AArnott]

Do you think it would be a good improvement to make it work at least when there are no locally registered async enumerables?

That wouldn't be enough. The constraint would have to be that there is exactly one remote target, since more than one remote target would recreate the same ambiguity problem.
But would supporting that be good? I'm thinking no. Right now it's a clear story: it doesn't work. But if we make it work for 1 remote, folks will build on that and likely expect they can add more remote targets or a local target and have it continue to work, and perhaps their architecture falls apart at that point because it can't grow with them.

We could allow calls to go to a remote target if the ID for the async enumerable is not found. IProgress<T> already works that way.

What do you mean IProgress<T> already works that way? That object only sends messages with the ID back to the client, and as there is only one client, the ID will always be unique. Am I missing something?
For async enumerable, the server tends to be the sender and if there are multiple senders and they are all incrementing their IDs from 1, there is a high probability of collision.

Personally, I am not interested in mixing local and remote targets, I am more interested in having remote targets only but intercepting and editing messages (or possibly dropping/injecting messages).

Well, that's another discussion. There is no support for intercepting messages today except by adding a local target object as well as the remote one, and having the local target object 'forward' messages by calling another JsonRpc object. And that would not work with your proposed 'only one remote target' idea.

Another option would be to allow (when multiple targets are supposed to be supported) to add a prefix

Sure, but the creator of the async enumerable would have to know the prefix, and the receiver would have to know it too in order to forward calls to the right one. So they'd have to agree on the prefix somehow, which isn't currently in the protocol. Not impossible, but challenging.

I guess we could do a very contrived thing here, which is to teach the middle-man the schema of each message enough that it can find the IDs in order to mutate them (and later route the messages sent to $/enumerable). But as this is probably a more general problem with the general object marshaling protocol as well, I think if we improve remote target objects support to deal with this, it should be a comprehensive fix.

[AArnott]

Here's an idea: we could add a setting to JsonRpc to suppress handling of $/ methods. If we don't handle them, they will become candidates for routing to the remote target object. Simple, and requires explicit opt-in so the developer realizes what the limitations would be.

[matteo-prosperi]

What do you mean IProgress already works that way?

$/enumerator/next is handled by JsonRpc.rpcTargetInfo.TryGetTargetMethod which results in the message being dropped if the token is unrecognized.

$/progress" is handled in JsonMessageFormatter.ReadRequest and MessagePackFormatter.JsonRpcRequestFormatter.Deserialize where the message is left to the normal handling if the token is unrecognized.

For reference this is an IProgress log

Sent:{"jsonrpc":"2.0","id":2,"method":"MethodWithProgressParameter","params":{"p":0}} // This is the token
Received:{"jsonrpc":"2.0","method":"$/progress","params":[0,1]} // These are the token and value

And the corresponding IAsyncEnumerable log.

Sent:{"jsonrpc":"2.0","id":2,"method":"GetNumbersAsync","params":[]}
Received:{"jsonrpc":"2.0","id":2,"result":{"token":1}} // This is the token
Sent:{"jsonrpc":"2.0","id":3,"method":"$/enumerator/next","params":[1]} // This is the token
Received:{"jsonrpc":"2.0","id":3,"method":"$/enumerator/next","params":[1]} // This is the value

[AArnott]

Interesting. I can't remember if there is a good reason for the difference in behavior, but silently ignoring based on an unrecognized token seems ... questionable.
Feel free to prepare a PR with the new JsonRpc setting to ignore the $/ properties as I mentioned.