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

Confusingly named ConnectionLostException thrown on client when client RPC instance is disposed prematurely #767

Open
gundermanc opened this issue Feb 14, 2022 · 3 comments

Comments

@gundermanc
Copy link
Member

Repro:

  1. Create JsonRpc instance, e.g.: perhaps via brokered service, servicehub service.
  2. Invoke long running request
  3. Dispose RPC instance

Expected:
Clear indication of what the client is doing wrong, such as ObjectDisposedException or message indicating that the client RPC instance was disposed.

Actual:
ConnectionLostException is thrown on the invocation Task. While this matches the documented behavior, it's initially unclear that the client is doing something wrong and it is not the server crashing or forcibly ending the session. As a result I spent a lot of time digging through irrelevant server logs trying to figure out the source of the crash.

Recommendations:
It seems like there would be benefit to having a way to differentiate between client-severed-connection and server-severed-connection, especially in fault telemetry.

@AArnott
Copy link
Member

AArnott commented Feb 15, 2022

While this matches the documented behavior

Really? I'm surprised to not find in this doc that this exception might be thrown. Where did you find this behavior documented?

I need to refamiliarize myself with the documented behavior to evaluate risk when changing that behavior. FWIW though, the docs do claim that InvokeAsync can throw ObjectDisposedException so what you're asking for seems like a reasonable idea.

@gundermanc
Copy link
Member Author

I was thinking specifically of this: https://github.com/microsoft/vs-streamjsonrpc/blob/716e9179a9895cd617b0c07046797578f450cc81/doc/disconnecting.md

[When] the JsonRpc.Dispose method is invoked ... [JsonRpc] faults all outstanding Task objects that represent outbound requests with a ConnectionLostException.

@AArnott
Copy link
Member

AArnott commented Feb 15, 2022

Ah, well that's pretty prescriptive there. Roslyn is pretty particular about the exception types we throw too, so changing this given it's documented there may have to be done with some coordination with them. The docs.microsoft.com web site is wrong either way and should be updated.

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

No branches or pull requests

2 participants