Repro Test for #3720 (Unobserved Exception) - #4460
Conversation
There was a problem hiding this comment.
Pull request overview
Adds unit-test infrastructure and a targeted repro test for issue #3720, demonstrating that exceptions thrown from SniPacket’s async IO completion callback can go unobserved and later surface via TaskScheduler.UnobservedTaskException.
Changes:
- Introduces a shared
ObservableExceptiontest utility to uniquely tag unobserved exceptions. - Refactors
AsyncHelperTestto use the sharedObservableExceptioninstead of an inline nested type. - Adds a new Managed SNI unit test that reliably reproduces the unobserved-exception behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/Utilities/ObservableException.cs | Adds a reusable exception type for unobserved-exception testing scenarios. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/Utilities/AsyncHelperTest.cs | Removes the inline ObservableException in favor of the shared utility type. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniPacketAsyncCallbackTest.cs | Adds a repro test for unobserved exceptions originating from SniPacket async callbacks. |
paulmedynski
left a comment
There was a problem hiding this comment.
Blocking on Copilot feedback. My suggestion about ObservableException is at your discretion.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniPacketTests.cs:47
- The unobserved-exception filter is currently using
InnerExceptions.Contains(testException), which relies on reference equality. This makes the test more brittle and also diverges from theObservableExceptionremarks and existingAsyncHelperTestpattern (match byIdentifier). Consider matchingObservableException.Identifierinstead (without LINQ, per prior discussion).
EventHandler<UnobservedTaskExceptionEventArgs> handleUnobservedException =
(_, args) =>
{
if (args.Exception.InnerExceptions.Contains(testException))
{
args.SetObserved();
unobservedExceptionRaised.TrySetResult(args.Exception);
}
};
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4460 +/- ##
==========================================
- Coverage 65.83% 62.59% -3.25%
==========================================
Files 287 283 -4
Lines 43763 67285 +23522
==========================================
+ Hits 28812 42114 +13302
- Misses 14951 25171 +10220
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Brings in the completed connection-pool pruning work (Story 2/3/4, #4463), which reworks PoolPruner to be driven by Connection Idle Timeout and only constructs a Pruner when IdleTimeout != 0. The single overlapping file, ChannelDbConnectionPool.cs, auto-merged cleanly: main's constructor pruner block coexists with this branch's ReplaceConnection additions. Also pulls in #4460 (unobserved-exception repro), #4347 (vector test refactor), and #4459 (pool benchmark coverage). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
This PR provides a reliable reproduction of #3720 in which an exception produced by the AsyncIOCompletionCallback of the SNI Packet class is not observed, leading to the unobserved exception handler being invoked.
This PR does not attempt to fix the issue, just adds a test that can be used to verify that the behavior exists. Attempts to fix the issue will inherently break this test, but can easily be fixed by changing the single assert in the test from "IsNotNull" to "IsNull".
Issues
#3720
Testing
Ran for hundreds of iterations with no failures observed.