-
Notifications
You must be signed in to change notification settings - Fork 560
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
#770 Fix unobserved SocketException #771
#770 Fix unobserved SocketException #771
Conversation
…asynchronous pattern (TAP) instead of Asynchronous Programming Model (APM) - fix unobserved SocketException
a66df9b
to
7ba1abb
Compare
_readCancellationTokenSource?.Dispose(); | ||
_readCancellationTokenSource = null; | ||
|
||
// just wait when read task will be cancelled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the unobserved SocketException
s were coming from closing the stream when disconnecting, and now we are cancelling the read with the CancellationTokenSource
, and the ContinueWith
is there to swallow the resulting OperationCanceledException
on the Task
. Is that right? Perhaps it is worth expanding the comment?
// just wait when read task will be cancelled | |
// The ContinueWith observes (and swallows) the resulting | |
// OperationCanceledException on the task. | |
// This avoids unobserved exceptions appearing elsewhere | |
// (which we ultimately don't care about during disconnect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Rob-Hague . I am sorry for the delayed response. Not exactly. The unobserved exception was triggered when we call .BeginRead(...)
and the other side just closed the connection. I reimplemented the socket reading based on Begin/End async approach to the task-based approach that will raise the exception on task awaits.
About the ContinueWith
you are absolutely right.
This looks promising, as do a few other recent PRs by you and others. I'm working to put out a new release right now. It will not include this, but I'm going to see if I can improve our process internally so we can start moving on a lot of these PRs for the next release after this (and have these releases come out on a more frequent bases). |
resolves connamara#770 (original issue) resolves connamara#771 (original PR, this is a rebase) Perform socket read operations according to Task-based asynchronous pattern (TAP) instead of Asynchronous Programming Model (APM)
#770 Perform socket read operations according to Task-based asynchronous pattern (TAP) instead of Asynchronous Programming Model (APM)