Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
  • Nov 24 08:28
    Kuling commented #195
  • Nov 24 08:27
    Kuling commented #195
  • Nov 23 20:25
    AArnott commented #195
  • Nov 23 16:36
    Kuling commented #195
  • Nov 23 15:38
    AArnott commented #195
  • Nov 23 13:34
    Kuling commented #195
  • Nov 15 02:22
    Tianfei-Wang commented #859
  • Nov 09 16:35
    CyrusNajmabadi commented #867
  • Nov 09 16:34
    AArnott commented #867
  • Nov 09 16:33
    AArnott closed #856
  • Nov 09 16:32
    CyrusNajmabadi commented #867
  • Nov 09 16:32
    AArnott commented #867
  • Nov 09 16:14

    AArnott on main

    Fix race condition in async enu… Merge pull request #869 from AA… (compare)

  • Nov 09 16:14
    AArnott closed #867
  • Nov 09 16:14
    AArnott closed #869
  • Nov 09 15:21
    AArnott review_requested #869
  • Nov 09 15:21
    AArnott review_requested #869
  • Nov 09 15:21
    AArnott review_requested #869
  • Nov 09 15:21
    AArnott milestoned #869
  • Nov 09 15:21
    AArnott milestoned #869
Roozbeh Gholizadeh
@roozbehid-ic
@AArnott Thanks. I figured out how to add prefix. It was a bit vague for me.
Here are something that are a bit confusing:
  • if AddLocalRpcTarget used with a class. It kind of flattens the class and uses its methods.
  • Any kind of method work with AddLocalRpcTarget, but for marshalled objects it seems they have to be ValueTask, Task, void. Why?
  • Using strong type, like interface also require that async nature, but calling with InvokeAsync works just fine
  • Properties does not work
Roozbeh Gholizadeh
@roozbehid-ic

Is an event like this would work? defined in an interface to be used with onRpc.Attach<T>() ?
event EventHandler<ActionReplyEventArgs> OnActionReply;

I am experience a crash inside ProxyGeneration.CreateParameterObjectType , line proxyTypeBuilder.CreateTypeInfo();

Rest of the interface is all ValutTask<t> or void. Although it is big like 200 methods.

Roozbeh Gholizadeh
@roozbehid-ic
I have a feeling I found the crash reason. methods with out parameter.
Do you think I should file an issue with that so ProxyGenerator just do an exception or something if it finds out interface has some "out" parameter. It can save time of some other people at least!
Roozbeh Gholizadeh
@roozbehid-ic
It seems even "in" in parameters of a method also causes a crash
Andrew Arnott
@AArnott

for marshalled objects it seems they have to be ValueTask, Task, void. Why?

Marshalled objects are marshalled by their interface, and their interface is shared with both client and server. Since RPC is fundamentally an async operation, that means the interface must be async. Contrast this with standard RPC target objects, where there is no interface contract with the client, the server may implement methods synchronously because the client can still invoke them asynchronously.

Using strong type, like interface also require that async nature, but calling with InvokeAsync works just fine

Same reason as the marshalled objects. We only expose async APIs to clients. You can use InvokeAsync or an interface if that interface exposes only async methods.

Properties does not work

Properties are not async, which is one reason why they are not allowed on RPC interfaces. But they are also properties -- not methods. And RPC is about invoking methods. Yes, we could pretend they are methods in the RPC protocol, but... you're more or less the first person to even suggest making them work, so it hasn't really been something on our radar to enable.

event EventHandler<ActionReplyEventArgs> OnActionReply;

Nothing strikes me as disallowed about that event. It seems that ought to work. Maybe file an issue on that with more context (a full repro would be ideal) and we can investigate.
nit: event names shouldn't have the On prefix, as that's used for event handlers.

Andrew Arnott
@AArnott

I have a feeling I found the crash reason. methods with out parameter.

Ya, the JSON-RPC protocol doesn't allow for out parameters. It sounds like we could throw a more helpful exception for that invalid case though. Yes please file an issue requesting a better exception in that case.

It seems even "in" in parameters of a method also causes a crash

We could make those work, but it's pretty pointless because the point of adding the in modifier is to pass readonly args by reference instead of by value, and in RPC, everything will be copied in a far more expensive way than any wins you'd typically get from adding an in modifier. So rather than fix the proxy generator to not throw in that case, I'd just throw a more helpful exception.

@roozbehid-ic, just calling your attention to my (late) replies.
Roozbeh Gholizadeh
@roozbehid-ic
Thanks for all detailed answers.
But on InvokeAsync I am still not sure if I fully understand. Right now you can call InvokeAsync on public static int SumOf(int a, int b) => a + b;as it was presented on help documents. It is not async but you can call it. So why this case is permitted ?
Andrew Arnott
@AArnott
@roozbehid-ic On the client everything is always async. The client sends a request, and the client is then done and can do other things. Eventually the response comes back and the client can process it. InvokeAsync reflects this.
When the server receives a request it dispatches it by executing the method. The server doesn't care whether the server method is implemented sync or async. Either way, when the method has completed execution, the server will send the response.
So by virtue of the client and server being totally separated except by I/O, one must be async, the other need not be.
But when the two of them share an interface, the more restrictive requirements of the client to be async get applied to the server because they must both implement the same interface.
The server doesn't actually have to implement the interface. The client could have just written an interface that matches the server's documented API, in which case the server could still be synchronously implemented while the client uses the interface to generate an async proxy. That works too.
Roozbeh Gholizadeh
@roozbehid-ic
Thanks. Now fully understand it!
Scott Harman
@scottharman
Hi guys - what would be the right way to reply to a server message from the client?
Vendor’s implementation sends a ‘ping’ every few seconds, and requires a corresponding ‘ping’ with the same id to come back again
1 reply
Scott Harman
@scottharman
Added to this, is the issue of errors - if I'm issuing an async request for data, but the parameter is incorrect, the interface signature will no longer match, and I'll get a result 0 with error text. What's the case for this? How do I extract the error to use it for error handling.
In these cases, there might be a result of 0, error: display ID unknown; or result 0, error: function not licensed
wardies
@wardies
Sorry, deleted my not-applicable reply. I don't think there should be a problem with generating your response. How did you try to define the method? Should work fine with something in your server like:
[JsonRpcMethod("ping")]
public string Ping() => "pong";
Scott Harman
@scottharman
Thats the issue I think - your original message might have been a better option
On my client I have event Eventhandler<string> ping; but if i had a raw payload i could intercept and construct a reply for this edge case
Scott Harman
@scottharman
Any thoughts on handling errors? Im only able to access the result that comes back, as being able yo collect a result and corresponding error if either are null would be useful
Andrew Arnott
@AArnott
@scottharman You're asking the wrong question (😉). There is no client or server in JSON-RPC. It's a peer-to-peer protocol. What you call the "server" is sending a request, and what you call the "client" is supposed to process and respond to the request. Setting up an event as you've been trying to do doesn't give you the opportunity to respond, because StreamJsonRpc uses events to handle notifications which have no responses.
Instead, you should configure your "client" like a server: provide an RPC target object that includes a ping method that returns pong.
4 replies

if I'm issuing an async request for data, but the parameter is incorrect, the interface signature will no longer match, and I'll get a result 0 with error text.

Then your server is faulty. If the parameter is incorrect and the signature does not match, you should not be getting a result object back at all. You should get an error back, with an error code of -32601 or -32602 (see the spec).
Given the server is providing a result instead of an error, you'll need to take up the issue with the server since their response is causing the difficulty.

1 reply

Any thoughts on handling errors? Im only able to access the result that comes back, as being able yo collect a result and corresponding error if either are null would be useful

StreamJsonRpc provides the result message as the result of the function call.
If the response is an error message, an exception is thrown that contains all the details from the error message.
Per the spec, no response message should have both an error and a result property, even if one is null. That would be invalid.

1 reply
Scott Harman
@scottharman

@scottharman You're asking the wrong question (😉). There is no client or server in JSON-RPC. It's a peer-to-peer protocol. What you call the "server" is sending a request, and what you call the "client" is supposed to process and respond to the request. Setting up an event as you've been trying to do doesn't give you the opportunity to respond, because StreamJsonRpc uses events to handle notifications which have no responses.
Instead, you should configure your "client" like a server: provide an RPC target object that includes a ping method that returns pong.

Hey @AArnott
So I have a half solution, but seem to have broken all strongly typed requests at the same time

                logger.Info($"Establishing connection to tcp://{NetworkAddress}:{TcpPort}");
                Stream networkStream = _tcpClient.GetStream();

                //var formatter = new JsonMessageFormatter(Encoding.Default);
                var handler = new NewLineDelimitedMessageHandler(networkStream, networkStream, new JsonMessageFormatter()); //empty formatter

                jsonRpc = new JsonRpc(handler, new JsonRpcProxyOptions { ServerRequiresNamedArguments = true });
                jsonRpc.Disconnected += ClientInterfaces_Disconnected;
                clientInterfaces= jsonRpc.Attach<ClientInterfaces>();
                jsonRpc.AddLocalRpcMethod("ping", new Func<string>(() => "pong"));

                clientInterfaces.MessageArrived += ClientInterfaces_MessageArrived;

So the ping now works, but any strongly typed methods now are treated as arrays (which the vendor's implementation errors out on) - the methods below all worked correctly prior to registering the new AddLocalRpcMethod - I'm suspecting it's as a result of attaching to the instance rather than the class, but not sure why.

        [JsonRpcMethod("handshake")]
        Task<SelectedVersion> HandshakeAsync(int[] client_supported_versions);
        [JsonRpcMethod("get_displays")]
        Task<Displays> GetDisplaysAsync();
        [JsonRpcMethod("get_layouts")]
        Task<Layouts> GetLayoutsAsync();
        [JsonRpcMethod("get_windows")]
        Task<Windows> GetWindowsAsync(DisplayContainer display);
        [JsonRpcMethod("get_display_inputs")]
        Task<Inputs> GetDisplayInputsAsync(DisplayContainer display);
Scott Harman
@scottharman

I have a wireshark trace to share if you prefer - but the handshake request is now:

request:
{"jsonrpc":"2.0","id":2,"method":"handshake","params":[[1,2]]}
reply
{"result": null, "error": {"code": -32602, "message": "Invalid parameters"}, "jsonrpc": "2.0", "id": 2}

before the change, my code was akin to clientInterfaces= JsonRpc.Attach<ClientInterfaces>(handler, new JsonRpcProxyOptions { ServerRequiresNamedArguments = true });

Wireshark

request:
{"jsonrpc":"2.0","id":2,"method":"handshake","params":{"client_supported_versions":[1,2]}}
reply:
{"error": null, "result": {"server_selected_version": 1}, "jsonrpc": "2.0", "id": 2}
wardies
@wardies

In order to support the named arguments, I believe you need the following attribute parameters defined in the server and switch to using a class for the return/parameter types:

[JsonRpcMethod("handshake", ClientRequiresNamedArguments = true, UseSingleObjectParameterDeserialization = true)]
public Task<SelectedVersionResult> HandshakeAsync(HandshakeParams handshakeParams);

Then you can define the result and params as follows:

public class SelectedVersionResult
{
    [JsonProperty("server_selected_version")]
    public int SelectedVersion { get; set; }
}

public class HandshakeParams
{
    [JsonProperty(PropertyName = "client_supported_versions")]
    public int[] ClientSupportedVersions { get; set; }
}
1 reply
In your test client you can use this to call the server:
var handshakeTask =
    jsonRpc.InvokeWithParameterObjectAsync<SelectedVersionResult>(
        "handshake",
        handshakeParams,
        cancellationToken);
Andrew Arnott
@AArnott
Thanks for offering, @wardies, but creating classes for request and result envelopes is not required. @scottharman, if you want named instead of positional arguments, you can just use [JsonRpcMethod("handshake", ClientRequiresNamedArguments = true)] on the interface method.
2 replies
Andrew Arnott
@AArnott
And BTW, just as a matter of history, the UseSingleObjectParameterDeserialization option is a bifurcation to the JSON-RPC protocol we had to add to support LSP, which is not exactly JSON-RPC, but awfully close.
wardies
@wardies
@AArnott Thanks, useful to know the UseSingleObjectParameterDeserialization option is not required here. Re: use of classes, although I see that the server method does not require this and does type matching, the use of classes seems to be required for the client to send the correct named parameters. In cases where the customer insists on receiving requests that exactly match their specification, I can't see a way to do this without classes. And since the server is not always ready during parallel development, we as developers need to produce both a server (test harness) and client, so it makes sense for our server and client to share the same classes (we add the class as a shared file link between projects).
Andrew Arnott
@AArnott

the use of classes seems to be required for the client to send the correct named parameters

Can you elaborate more on this? I can't think of anything that you can describe with fields on a class that you can't describe with method parameters.

I think I made a mistake when I copied ClientRequiresNamedArguments over from another poster's code. That property doesn't exist on JsonRpcMethodAttribute now that I look. Instead, the property I was thinking of to force JsonRpc to send requests with named arguments instead of positional ones is JsonRpcProxyOptions.ServerRequiresNamedArguments. So yes, you can achieve named arguments with interfaces and proxies, but without a special request class.

it makes sense for our server and client to share the same classes (we add the class as a shared file link between projects).

We accomplish this on my team by sharing the RPC interface, which defines the methods and parameters on them.

wardies
@wardies
I admit I had totally misunderstood the purpose of the jsonRpc.Attach<interface> call; I thought it was registering the callback interface so that StartListening() knew what to listen out for, but now I recognise that StartListening() is sufficient if the this class referenced in var jsonRpc = new JsonRpc(clientSocket, this) implements a method with a [JsonRpcMethod] property having matching name (where the property definition may exist directly on the method implementation, or on the interface method if the class implements that interface). This is making sense now, however...
wardies
@wardies

@AArnott If I define the interface method as

[JsonRpcMethod("handshake", ClientRequiresNamedArguments = true, UseSingleObjectParameterDeserialization = true)]
public Task<SelectedVersionResult> HandshakeAsync(HandshakeParams handshakeParams);

Then there seems to be only one way I can call that successfully, namely using the InvokeWithParameterObjectAsync method on the JsonRpc instance:

var handshakeTask =
    jsonRpc.InvokeWithParameterObjectAsync<SelectedVersionResult>(
        "handshake",
        handshakeParams,
        cancellationToken);

Which results in the desired JSON-RPC call:

{"jsonrpc":"2.0","id":5,"method":"handshake","params":{"client_supported_versions":[1,4,6]}}
wardies
@wardies

If I try to call it through the attached client interface:

var clientToServer = jsonRpc.Attach<IDemoServer>(new JsonRpcProxyOptions
{
    ServerRequiresNamedArguments = true
});
var handshakeTask = clientToServer.HandshakeAsync(handshakeParams)
    .WithCancellation(cancellationToken);

The handshakeParams parameter name gets added to the JSON-RPC call:

{"jsonrpc":"2.0","id":5,"method":"handshake","params":{"handshakeParams":{"client_supported_versions":[1,4,6]}}}

Which results in the method on the server side getting called but all of the class fields being null. Is this by design? If so, it's not a problem, we will just continue using the InvokeWithParameterObjectAsync method.

wardies
@wardies

the use of classes seems to be required for the client to send the correct named parameters

Can you elaborate more on this? I can't think of anything that you can describe with fields on a class that you can't describe with method parameters.

Yes, I can see now that after attaching the client proxy conforming to the server interface, it is possible to send the desired one or more top-level named parameters. It seems a bit less ideal for our purposes because the server method implementation needs to use the exact same parameter names as the interface method definition parameter names, whereas if we use a single class parameter (with multiple properties and nested classes) we can define all the JsonProperty names in one place.

A minor benefit is avoiding the inevitable variable/parameter naming style warnings which could trick an unsuspecting developer into renaming client_supported_versions to clientSupportedVersions in one or more places during a review. But it's still useful to know this is a viable alternative. Thanks; I really appreciate your thoughtful responses, allowing me to retrace my steps, rethink my incorrect assumptions and learn more along the way!

wardies
@wardies
... and yes, a potential solution to over-eager developer review would be to strategically distribute appropriate comments such as
[JsonRpcMethod("handshake", ClientRequiresNamedArguments = true)]
// ReSharper disable once InconsistentNaming  Do not rename this parameter! See the customer's JSON-RPC spec
public Task<SelectedVersionResult> HandshakeAsync(int[] client_supported_versions);
Jonathan Hjertström
@nixi

I am at the beginning of using this great software. I have devices that sends data similir to this:

request:
{ "jsonrpc": "2.0", "id": 888, "method": "SayHello", "params": { "Name": "Jonathan" } }
reply:
{ "jsonrpc": "2.0", "id": 888, "error": { "code": -32602, "message": "Unable to find method 'SayHello/1' on {no object} for the following reasons: An argument was not supplied for a required parameter." } }

It will not reqonice the parameter "Name" and its value.

JsonRpcServer2.cs:

public Task<HelloReply> SayHello(HelloRequest request) { 
    return Task.FromResult(new HelloReply() { Message = "Hello " + request.Name }); 
}

StreamJsonRpcMiddleware.cs

JsonRpc rpc = new JsonRpc(jsonRpcMessageHandler);
rpc.AddLocalRpcTarget( new JsonRpcServer2());
rpc.TraceSource = traceSource;
rpc.StartListening();
await rpc.Completion;

If I enclose the params with [] it works. I have howwever no control of the incoming data.

request:

{"jsonrpc":"2.0","id":888,"method":"SayHello","params":[{"Name":"Jonathan"}]}
reply:
{"jsonrpc":"2.0","id":888,"result":{"Message":"Hello Jonathan"}}

Hopefully you will laugh and correct my mistake or do I have a problem?

11 replies
wardies
@wardies
@nixi Works for me if I add the following before the SayHello() task definition:
[JsonRpcMethod(UseSingleObjectParameterDeserialization = true)]
public Task<HelloReply> SayHello(HelloRequest request);
Andrew Arnott
@AArnott

ClientRequiresNamedArguments = true

@wardies You keep using that. Do you know what it does? It refers to callbacks that you pass in as arguments to that method. I'm guessing your HandshakeParams doesn't include any such callbacks (such as IProgress<T>), so I think your use of this attribute property is pointless and should be removed.

wardies
@wardies
I don't know why it works, I am in the dark... happy to be shown a better way!
Andrew Arnott
@AArnott

Then there seems to be only one way I can call that successfully

You could also invoke it with

jsonRpc.InvokeAsync<SelectedVersionResult>("handshake", new [] { 1,4,6 });

This would use positional args instead of named parameters, but it would be able to invoke HandshakeAsync, at least if you have defined order on the fields in your HandshakeParams class.

If I try to call it through the attached client interface:

You have to change the RPC interface in that case, because the interface declares a method that takes one parameter called handshakeParams, and you stated in the Attach<T> method that you want all arguments to be passed as named arguments, that means that handshakeParams is itself a named argument, which doesn't match what your receiving end is likely expecting.

wardies
@wardies

This would use positional args instead of named parameters, but it would be able to invoke HandshakeAsync, at least if you have defined order on the fields in your HandshakeParams class.

Yes, I was able to confirm that works, but it's difficult going from a mindset where everything is defined by classes to positional arguments... it doesn't seem quite as safe or clean.

Andrew Arnott
@AArnott
Maybe this mindset will help: JSON-RPC is a remote procedure call language. It's about invoking methods, and passing arguments to them, and getting a result back. Methods take parameters, so you pass in parameters. You wouldn't change every single method in your program to take exactly one parameter, would you? Of course not. So why do it in the RPC case?
2 replies
Andrew Arnott
@AArnott

So here's how I'd set it up (I'm guessing at some of your protocol requirements since you've left some undefined):

First, the interface, which I'd tend to share as source or assembly across both parties:

public interface ISomeService
{
   Task<int> HandshakeAsync(int[] client_supported_versions, CancellationToken cancellationToken);
}

Implementing this is of course straightforward. If it doesn't need to be implemented asynchronously, just use return Task.FromResult(3); instead of return 3;. We declare it as async to conform to the interface, which makes sharing the RPC protocol across both parties easy.

class SomeService : ISomeService
{
   public Task<int>  HandshakeAsync(int[] client_supported_versions, CancellationToken cancellationToken) => Task.FromResult(client_supported_versions[0]);
}

JsonRpc connection = JsonRpc.Attach(ioStream, new SomeService());
await connection.Completion;

Server is all done. Now the client can call with:

ISomeService clientProxy = JsonRpc.Attach<ISomeService>(ioStream);
using (clientProxy as IDisposable)
{
   int result = await clientProxy.HandshakeAsync(new int[] { 1, 2, 3 });
}

Again, super simple.

Now, if you have StreamJsonRpc on both sides, you're done. But if you have to take care to match an existing JSON-RPC party, then extra steps can be taken. First, maybe you want the RPC method name to be handshake. You can leverage name transforms to do this for all methods at once. But for this sample, let's just say we stick [JsonRpcMethod("handshake")] on the interface declaration of the method and we're done.

Now consider that maybe you have an existing server that demands the request be sent with named arguments instead of positional ones. No problem. Just change the construction of the client proxy to this:

ISomeService clientProxy = JsonRpc.Attach<ISomeService>(ioStream, new JsonRpcProxyOptions { ServerRequiresNamedArguments = true });

And now you're done. A StreamJsonRpc server will always support both positional and named arguments, but the above change will allow a client to accommodate a lesser server that only supports one of these.

Scott Harman
@scottharman

Hey @AArnott and @wardies - thanks so much for all your help!
Much progress has been made
... now it's the edge and corner cases -(I regret typing that statement, and now immensely regret it)

how can I send an explicit null i.e. display { id: 1} ,layout {id: null} - currently whether I use an anonymous class or named objects, the null is always replaced with an empty payload

{"jsonrpc":"2.0","id":47,"method":"set_display_layout","params":{"display":{"id":1},"layout":{}}}
wardies
@wardies
@scottharman Should be as simple as defining the following attribute on your class property:
[JsonProperty("id", NullValueHandling = NullValueHandling.Include)]
public int? Id { get; set; }
wardies
@wardies
Worth remembering that the above assumes serialization is being done by Newtonsoft.Json and others are available; ref. the chosen IJsonRpcMessageFormatter -- see https://github.com/microsoft/vs-streamjsonrpc/blob/main/doc/extensibility.md
Andrew Arnott
@AArnott
@scottharman are you saying that when you set the layout argument to null, it is serialized as "layout":{} instead of "layout": null? If so, I have no idea why that would happen. That seems broken. But as @wardies advises, this is likely due to newtonsoft.json serialization, so you might try defining a class that contains the same layout as a field, and serialize that yourself to see what you get.