Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
AArnott opened this issue Jan 11, 2022 · 4 comments

Comments

@AArnott
Copy link
Member

AArnott commented Jan 11, 2022

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 AArnott added the bug label Jan 11, 2022
@AArnott
Copy link
Member Author

AArnott commented Jan 11, 2022

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

@RyanToth3
Copy link
Member

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
Copy link
Member

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
Copy link

Adding @tmat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants