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
750 stars 149 forks source link

Events raised by client proxy should set `sender` to proxy rather than JsonRpc object #755

Open AArnott opened 2 years ago

AArnott commented 2 years ago

The dynamic proxies generated on the client raise events with sender set to this.rpc (the JsonRpc object behind the proxy). For a proxy holder, particularly one that has never seen the JsonRpc object because it was created by another party, this may be unexpected.

I think the proxy sending this as the sender parameter is more appropriate so that someone's event handler can find the object on which the handler was added via the sender argument.

AArnott commented 2 years ago

@sharwell @RyanToth3 @CyrusNajmabadi @BertanAygun Do any of you think fixing this would break your existing code?

RyanToth3 commented 2 years ago

We don't use a whole lot of event handlers in our RPC Contracts, in ServiceHub or elsewhere. After a quick search I don't see anywhere where this would affect me.

I do see however that in the AvailabilityChanged event handlers of a lot of the ServiceBroker classes that you've already overriden this behavior and done what you're describing above: https://devdiv.visualstudio.com/DevDiv/_git/DevCore/?path=src/clr/Microsoft.ServiceHub.Framework/RemoteServiceBroker.cs&version=GC3a218cd53dc99fb6e86d497bf2a475e28bd5b7c0&lineStyle=plain&line=611&lineEnd=611&lineStartColumn=157&lineEndColumn=112

BertanAygun commented 2 years ago

I have an event handler in one of the RPC contracts I own but the only callers I know don't rely on "sender" parameter so this should be fine from my point of view as well.

CyrusNajmabadi commented 2 years ago

Adding @tmat