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

Stabilize Dispose patterns #246

Closed
AArnott opened this issue Mar 7, 2019 · 3 comments · Fixed by #538
Closed

Stabilize Dispose patterns #246

AArnott opened this issue Mar 7, 2019 · 3 comments · Fixed by #538
Assignees
Milestone

Comments

@AArnott
Copy link
Member

AArnott commented Mar 7, 2019

Customers have reported a few times that they want to Dispose of JsonRpc (or indirectly its MessageHandlerBase):

  1. from their Disconnected event handler
  2. while outbound messages are still in progress or perhaps even in transmission.

We know that disposing the MessageHandlerBase while messages are being transmitted is very problematic (e.g. dotnet/roslyn#18563).
And I suspect disposing from our event handler has undefined behavior.

  1. I think we should add a test for the disposal from our event handler if we can make that work (for user convenience).
  2. We should stop disposing our AsyncSemaphore in MessageHandlerBase.Dispose. There are no native resources to release, and this would avoid random exception types being thrown on our transmission thread, sometimes leading to crashes. Instead, we can set a boolean field so that any future transmission request is denied with an ObjectDisposedException.

Also: when is ObjectDisposedException thrown compared to ConnectionLostException? Even OperationCanceledException can be thrown back to pending requests due to a disconnection.

@AArnott AArnott added this to the v2.0 milestone Mar 7, 2019
@AArnott AArnott self-assigned this Mar 7, 2019
@AArnott
Copy link
Member Author

AArnott commented Mar 7, 2019

I've re-opened microsoft/vs-threading#105 where we might place the fix for no. 2 above.

AArnott added a commit to AArnott/vs-streamjsonrpc that referenced this issue Mar 9, 2019
@AArnott AArnott modified the milestones: v2.0, v2.1 Jun 11, 2019
@AArnott
Copy link
Member Author

AArnott commented Oct 18, 2019

No. 2 above is indeed resolved by microsoft/vs-threading#493

@tmat
Copy link
Member

tmat commented Sep 1, 2020

This would be very useful. We need to get to a state when remote calls can only throw 4 types of exceptions that correspond to

  1. Connection issue (connection dropped for any reason)
  2. Serialization issue - bug in serialization of arguments (types are not serializable, etc.)
  3. Remote exception - an exception was thrown by the callee
  4. Cancelation

Also, when a connection is dropped and CancelLocallyInvokedMethodsWhenConnectionIsClosed is set the connection dropped exception [1] should not be thrown. Instead a the cancellation token should be signaled and OperationCancelledException should be thrown ([4]).

@AArnott AArnott added this to the v2.6 milestone Sep 2, 2020
AArnott added a commit to AArnott/vs-streamjsonrpc that referenced this issue Sep 2, 2020
Everything checks out, but this resolves a concern brought up in microsoft#246
matteo-prosperi pushed a commit that referenced this issue Feb 5, 2024
Co-authored-by: Andrew Arnott <andrew.arnott@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants