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

Fix unobserved SocketException (rebase of PR 771, which addresses Issue 770) #836

Conversation

gbirchmeier
Copy link
Member

resolves #770 (original issue)
resolves #771 (original PR, this is a rebase)

Perform socket read operations according to Task-based asynchronous pattern (TAP) instead of Asynchronous Programming Model (APM)

@gbirchmeier gbirchmeier marked this pull request as draft February 14, 2024 22:44
@gbirchmeier
Copy link
Member Author

@nmandzyk and @Rob-Hague -

I rebased the original PR #771 on latest. SocketReader now has null-analysis turned on, and the compiler is flagging that _readCancellationTokenSource can be null at the start of ReadSome().

Looking into that has led me to DisconnectClient(), which... well, is there a reason to assign _readCancelationTokenSource and _currentReadTask to null here? If we left _readCancellationTokenSource as a disposed non-null object, the logic wouldn't change, and then we could take the ? off the var declaration so it would never be null and the warning in ReadSome() would go away.

(If I do this, and by some weird chance the var gets referenced after DisconnectClient(), then we'd now get a ObjectDisposedException instead of a NullRefException. Barely a difference functionally, and probably a little easier to diagnose if we need to follow up on it).

(I suspect SocketInitiatorThread might have a similar issue, but I'll enable nullable on this file and clean it up in another commit.)

@gbirchmeier
Copy link
Member Author

I've made the nullable fix in the second commit. Please tell me what you think.

Copy link
Contributor

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nullable changes look good. I've left a few comments on SocketReader, although they mostly pertain to the original PR. TBH the benefit/risk ratio on this PR doesn't feel super high

QuickFIXn/SocketReader.cs Outdated Show resolved Hide resolved
QuickFIXn/SocketReader.cs Outdated Show resolved Hide resolved
QuickFIXn/SocketReader.cs Show resolved Hide resolved
QuickFIXn/SocketReader.cs Show resolved Hide resolved
QuickFIXn/SocketReader.cs Outdated Show resolved Hide resolved
@gbirchmeier
Copy link
Member Author

I made those changes.

I have also nullable-ized and cleaned up SocketInitiatorThread, and also replicated the similar changes in that file. All tests appear passing, and I would again appreciate a pair of eyes to review these changes.

Copy link
Contributor

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

QuickFIXn/SocketInitiatorThread.cs Show resolved Hide resolved
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)

Also cleanup/nullable-ize SocketInitiatorThread (this part by @gbirchmeier)
@gbirchmeier gbirchmeier force-pushed the pr771-nmandzyk-770_unobserved_exceptions branch from 6b5b278 to 76cbf8f Compare February 21, 2024 22:53
@gbirchmeier gbirchmeier marked this pull request as ready for review February 21, 2024 23:13
@gbirchmeier gbirchmeier merged commit 0ed0129 into connamara:master Feb 21, 2024
2 checks passed
@gbirchmeier gbirchmeier deleted the pr771-nmandzyk-770_unobserved_exceptions branch February 21, 2024 23:14
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

Successfully merging this pull request may close these issues.

Unobserved exception for socket read operations
3 participants