Support batching about vs-streamjsonrpc HOT 11 CLOSED

The Server should respond with an Array containing the corresponding Response objects, after all of the batch Request objects have been processed.

This particular requirement from the spec seems difficult to achieve. Given we have no one asking for it, I'm closing this as Won't Fix.

from vs-streamjsonrpc.

Reactivating as this might be a better solution for servers that must be able to process a "batch" of requests from a client in-order. For example, we might author the server-support such that when a batch of requests come in, we dispatch them one-at-a-time so the server can't possibly mix up their order.

from vs-streamjsonrpc.

Reactivating

You don't seem to have reactived this :)

Note: this is currently an issue for Roslyn as @dibarbet can attest to. Specifically, we are forced to create our own one-off sync context just to ensure that we can serialize requests in a reasonable order. Even 'await'ing the request has no effect here. This is highly unintuitive and seems to break reasonable expectations around how await and channels would work. i.e. i would always expect that if i have these calls:

await SendMessageToChannel("a")
await SendMessageToChannel("b")

That those messages always happen in order on the other end. That would be the expected default behavior. And only clients who specifically didn't want that could opt out.

note that i don't even understand the case of people wantin to opt out of this. If you wanted opt out, you could always just write:

await Task.WhenAll(SendMessageToChannel("a"), SendMessageToChannel("b"))

Any chance this could get attention @AArnott ?

from vs-streamjsonrpc.

Hi @CyrusNajmabadi

Yes, my last comment does look weird since I didn't reactivate. That said, I don't think we have a need for batching support yet. And forcing a batching concept on callers to ensure message ordering is too onerous on the user, IMO. So ordering should Just Work as you described.

I first want to refer you (and anyone else seeing this) to this topic that describes message ordering rules.

You gave the example that doing this doesn't ensure ordering:

await SendMessageToChannel("a");
await SendMessageToChannel("b");

I guess that depends on what SendMessageToChannel does. Certainly we do preserve order of requests when you use JsonRpc or the proxies that are generated by this library. So this absolutely does send requests in the order you specify:

jsonRpc.InvokeAsync("SomeMethod1");
jsonRpc.InvokeAsync("SomeMethod2");

Note you don't even have to await them. The order is locked in by the order of calls into InvokeAsync without regard to awaiting. This is a guarantee we make.

That's request order. Now let's look at the order in which RPC methods get invoked on the server side:

By default, we execute incoming RPC calls on the threadpool, but a SynchronizationContext may be supplied so that we invoke RPC calls on whatever context you give us. This allows you to invoke RPC methods on the application's main thread if that's what you want. It also allows you to invoke them on the threadpool, but in order instead of the threadpool "queue"'s unordered scheduler.

But going back to your example, if you're sending requests and awaiting responses between each one, there's no way the methods can be invoked out of order. Just think about it: until the server sends a response, you can't even send the next request. Now if you're sending JSON-RPC notifications instead of requests, such that the client doesn't wait for the server to acknowledge the message, then yes the server may invoke the methods out of order if it doesn't supply an order-preserving SynchronizationContext.

Also note that I filed #440 just earlier today so that we have an in-box solution to turning on message ordering.

I'm primarily interested in your response to the above.

But as an aside, I'll respond to this:

That would be the expected default behavior.

I might argue that point, since JsonRpc's primary job is not synchronization of some object but rather remoting of calls: if you make multiple calls of a local object without awaiting or on multiple threads you have the same ordering problem. JsonRpc isn't there to fix that.

Applying an order preserving SyncContext would be a breaking change for some too, if the first invocation synchronously blocks until a second invocation comes in. So I would be hesitant to change the default even if you could convince me that the other behavior is the better default.

from vs-streamjsonrpc.

Now if you're sending JSON-RPC notifications instead of requests, such that the client doesn't wait for the server to acknowledge the message, then yes the server may invoke the methods out of order if it doesn't supply an order-preserving

This just seems odd to me. We're awaiting the call on our end. But it's executing out of order on the other end. Is it possible to disable 'notifications'? We have no desire for any sort of message sent to the server that won't execute in the same order that the individual calls serialize out locally.

I might argue that point, since JsonRpc's primary job is not synchronization of some object but rather remoting of calls: if you make multiple calls of a local object without awaiting or on multiple threads you have the same ordering problem. JsonRpc isn't there to fix that.

But i guess that's my point. We are awaiting. We're not on multiple threads.

--

Finally, i've used many different RPC systems over the years at several different companies. I've never seen any of them behave this way (by default). Even thrift (one of my least favorite) requires you to explicitly ask for this behavior.

These behaviors just seem counterintuitive to me (similar to how TPL will let tasks run before their antecedent tasks have all finished running by default). Programmers are used to procedural serialized code. IMO, abstractions should default to that behavior, and ask users to opt into these more complex concurrency behaviors if they want it. That's what we did with async/await for example. It very much was designed that someone should be able to use their procedural intuition for how it works.

I get that you likely can't cahnge this now. But i would def +1 on adding a knob/switch to only get this behavior through explicit action on our part.

from vs-streamjsonrpc.

But it's executing out of order on the other end. Is it possible to disable 'notifications'? We have no desire for any sort of message sent to the server that won't execute in the same order that the individual calls serialize out locally.

No, you can't disable them. But you have to opt into them in the first place. You haven't shared your client code yet, but if you did, you'll see it's ultimately either a call to the JsonRpc.InvokeAsync family or the JsonRpc.NotifyAsync family of methods. Notify is a fire-and-forget style, so when you await the call, you're only awaiting its transmission. But the InvokeAsync family of methods is request-response style, so you're awaiting the server's completion of the invocation.
Thus, if you await every single call, InvokeAsync guarantees you'll execute one method at a time, in order. While awaiting each NotifyAsync only guarantees that you don't flood your own outbound message queue.
Either way, the server will always receive the messages in order, but since notifications could pile up, if the server simply dispatches all incoming calls to the threadpool, then they can run concurrently or in other orders.

But i guess that's my point. We are awaiting. We're not on multiple threads.

Then I tell you: the order is not getting mixed up -- or you're using notifications, which is something you had to opt into and I guess isn't the right choice in your case given what you're trying to do.

I've never seen any of them behave this way (by default)

A little history then to help you understand how we came to be here: when StreamJsonRpc was new, it wasn't used for a stateful protocol like LSP. We used it for ServiceHub, which had a stateless protocol where we just fired of requests that we wanted filled as soon as possible. Thus concurrency was both compatible and welcome.
It was only when LSP came along that we even had to think about preserving order. We made some changes to guarantee order of transmitting request and of receiving requests, up to scheduling the invocation, since we could do all that without a breaking change or reducing throughput. We left the "I'm willing to sacrifice throughput to preserve order" switch as opt-in though.

Note too, that even with an order preserving SynchronizationContext, if your RPC methods are async, the next invocation is allowed as soon as the last one yields. We do not provide any way to serialize invocations of async methods such that they aren't at least interleaved with each other. Your SynchronizationContext certainly can prevent concurrency, but the async continuations of each method may still interleave. The only way to block that is to make sure your RPC methods are synchronous. I wonder if that bothers you.

from vs-streamjsonrpc.

Lets move this conversation to #440 since it's actually active and is meant to discuss ordering.

from vs-streamjsonrpc.

Ah.

you'll see it's ultimately either a call to the JsonRpc.InvokeAsync family or the JsonRpc.NotifyAsync family of methods. Notify is a fire-and-forget style, so when you await the call, you're only awaiting its transmission. But the InvokeAsync family of methods is request-response style, so you're awaiting the server's completion of the invocation.

@dibarbet Is this what's going on? Can we move to InvokeAsync instead of NotifyAsync? That seems like it would solve our issues here.

from vs-streamjsonrpc.

Hi!

I'm actually running into this issue - using this library to communicate with a server which may at times send out batch notifications (messages are newline delimited).

Example server message:
[{"jsonrpc":"2.0","method":"Client.OnVolumeChanged","params":{"id":"xx:xx:xx:xx","volume":{"muted":true,"percent":32}}},{"jsonrpc":"2.0","method":"Client.OnVolumeChanged","params":{"id":""xx:xx:xx:xx","volume":{"muted":false,"percent":70}}}]

Exception:

JsonRpc Critical: 13 : Connection closing (StreamError: Reading JSON RPC from the stream failed with ArgumentException: Accessed JArray values with invalid key value: "jsonrpc". Int32 array index expected.). System.ArgumentException: Accessed JArray values with invalid key value: "jsonrpc". Int32 array index expected.
   at Newtonsoft.Json.Linq.JArray.get_Item(Object key)
   at Newtonsoft.Json.Linq.JToken.Value[T](Object key)
   at StreamJsonRpc.JsonMessageFormatter.Deserialize(JToken json) in C:\STM\Snap.Net\ThirdParty\vs-streamjsonrpc\src\StreamJsonRpc\JsonMessageFormatter.cs:line 364
   at StreamJsonRpc.JsonMessageFormatter.Deserialize(ReadOnlySequence`1 contentBuffer, Encoding encoding) in C:\STM\Snap.Net\ThirdParty\vs-streamjsonrpc\src\StreamJsonRpc\JsonMessageFormatter.cs:line 301
   at StreamJsonRpc.JsonMessageFormatter.Deserialize(ReadOnlySequence`1 contentBuffer) in C:\STM\Snap.Net\ThirdParty\vs-streamjsonrpc\src\StreamJsonRpc\JsonMessageFormatter.cs:line 293
   at StreamJsonRpc.NewLineDelimitedMessageHandler.<ReadCoreAsync>d__12.MoveNext() in C:\STM\Snap.Net\ThirdParty\vs-streamjsonrpc\src\StreamJsonRpc\NewLineDelimitedMessageHandler.cs:line 157

from vs-streamjsonrpc.

@stijnvdb88 I recently wrote a sample extension to StreamJsonRpc that enables receiving batched JSON-RPC messages to solve the problem you're facing. You can see it here: main...AArnott:sampleUnbatchingMessageHandler

Feel free to copy and paste into your own project.

from vs-streamjsonrpc.

That's exactly what I was looking for, thanks a lot!

from vs-streamjsonrpc.