microsoft / vs-streamjsonrpc

The StreamJsonRpc library offers JSON-RPC 2.0 over any .NET Stream, WebSocket, or Pipe. With bonus support for request cancellation, client proxy generation, and more.
Other
747 stars 149 forks source link

JoinableTaskFactory bridge fails with multiple JsonRpc instances in a process without JTF #983

Closed AArnott closed 9 months ago

AArnott commented 10 months ago

The following is a hypothesized bug based on a deadlock investigation in VS. It has not yet been confirmed. We should write unit tests for all the scenarios described here to confirm the hypothesis and then fix the bug, if necessary.

When process A that uses JTF calls out to process B that does not use JTF, the JsonRpc object that receives the request observes the JoinableTask 'cookie' that process A sent and stores it in an AsyncLocal instance field on the JsonRpc object. If in processing the dispatched request, the RPC server makes an outbound call, that cookie goes with it so that process A, in receiving that request, can know that its relevant to the request it sent, and then connect the two JTF contexts into one, mitigating potential deadlocks if the original request from A is blocking its own main thread, and the incoming request to A needs the main thread.

However there is a significant design limitation in how this works, such that the deadlock is only mitigated if process B makes its own outbound call on the same JsonRpc object that dispatched the incoming request. But this isn't always the case. In fact if process B is using brokered services, it's very likely to make calls back into process A on a different JsonRpc object, which still results in a deadlock.

If process B were using JTF and passed that to both JsonRpc objects, I believe this would not be an issue because JsonRpc would have created a JoinableTask for the inbound call, which would propagate to the outbound call. But we should verify this. The problem though is suspected to at least apply to non-JTF processes, where no such coupling between JsonRpc objects exist. For such cases, it seems we'll need some way for these objects to collaborate on their AsyncLocal field.

BertanAygun commented 10 months ago

A challenge of doing this at StreamJsonRpc layer would be separating out related services with in a process with the assumption that we would not want to assume unrelated services would be using the same JTF context. The assumption holds true for now for VS service hub but I don't know if we want to rely on that behavior globally.

One option I was wondering is if we can associate a JoinableTaskContext per factory instance in service hub and attach it to descriptors returned by the factory. We would also have to ensure this instance is also associated with JsonRpc's created while IServiceBroker is used from servicehub process.

AArnott commented 9 months ago

I have the test that demonstrates the problem.

BertanAygun commented 9 months ago

I approved the PR, it looks like it should allow us to have a tracker per service factory in service hub/extensibility host.

AArnott commented 9 months ago

Yes, it should allow that, though I can't think of why we would want to isolate groups of JsonRpc instances, actually. AsyncLocal<T> itself will generally do the right thing as it does in devenv.exe, I think.