[HttpStress] Track unobserved exceptions #114225
Conversation
|
/azp run runtime-libraries stress-http |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the HttpStress test harness by tracking unobserved exceptions and reporting their occurrence without causing test failures. It introduces a new command line option, a dictionary to store exception occurrences, and updates the configuration accordingly.
- Added a static dictionary to track unobserved exceptions.
- Registered an event handler for TaskScheduler.UnobservedTaskException.
- Provided a new CLI option and configuration property to control this behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/StressTests/HttpStress/Program.cs | Added tracking for unobserved exceptions and printed a summary report |
| src/libraries/System.Net.Http/tests/StressTests/HttpStress/Configuration.cs | Added a nullable boolean property to enable or disable exception tracking |
| { | ||
| lock (s_unobservedExceptions) | ||
| { | ||
| string text = e.Exception.ToString(); |
There was a problem hiding this comment.
Consider using a more concise identifier for exceptions (e.g. exception type and message) instead of the full output of ToString(), to avoid using overly verbose keys in the dictionary and potential memory overhead.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
The CI test run will show if the overhead is high. Even if that's the case, fixing #114128 should significantly reduce it.
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Tagging subscribers to this area: @dotnet/ncl |
| @@ -210,10 +230,28 @@ private static async Task<ExitCode> Run(Configuration config) | |||
| client?.Stop(); | |||
| client?.PrintFinalReport(); | |||
|
|
|||
| if (trackUnobservedExceptions) | |||
| { | |||
| PrintUnobservedExceptions(); | |||
There was a problem hiding this comment.
Should we consider stress as failed if we see any? Just so we don't ignore them by accident.
There was a problem hiding this comment.
My plan is to monitor the results after #114226 first to make sure we don't add too much noise here.
|
/ba-g CI failures are unrelated |
This is a variant of #105336/#111126 I actually intend to merge, assuming it won't kill CI.
With this PR the presence of such exceptions is only informative and doesn't fail the test. If addressing #114128 (comment) would bring them to zero after multiple runs, we can revisit this decision.