diagnostics icon indicating copy to clipboard operation
diagnostics copied to clipboard

Change SocketException to ServerNotAvailable + misc test fixes/logging

Open noahfalk opened this issue 9 months ago • 1 comments

This started out as a bit of test logging. Then the unrelated GetProcessInfo test failed in CI so the change grew into a small product fix + more test improvements.

Product changes:

  • Connecting to the IPC channel might fail when the runtime is starting up, shutting down, or is overloaded. If so the socket.Connect() call is going to throw a SocketException. In order to avoid having callers to DiagnosticClient APIs understand all the implrementation details and have to use catch clauses for a wide variety of different exceptions we now catch this exception and rethrow it wrapped as a ServerNotAvailableException.

Test changes:

  • Add logging to the DuplicateMetricNames test to improve diagnostics when it hangs.
  • Add retries to the GetProcessInfo requests that are running while the target process is starting up. There is no guarantee the runtime is listening to the socket at this point and connection requests may be refused. This may resolve one cause of flakiness noted in https://github.com/dotnet/diagnostics/issues/2760. The issue also shows a 'ReadPastEndOfStream' problem that is not addressed by this fix.
  • Add verification that the ExePath returned by DebuggeeCompiler actually exists on disk. This catches some build/config issues in the test setup more eagerly.
  • Add verification that the BuildProjectFramework and TargetConfiguration used by SdkPrebuiltDebuggeeCompiler are populated. This catches some specific test configuration errors more eagerly.
  • Tweaked some out-of-date comments

noahfalk avatar Mar 18 '25 07:03 noahfalk

PTAL @dotnet/dotnet-diag

noahfalk avatar Mar 19 '25 07:03 noahfalk

@noahfalk still good to merge?

steveisok avatar Jul 12 '25 18:07 steveisok

I just accepted Mitchell's suggestion but it reset the review gate. But if someone can accept again - yep ready to merge!

noahfalk avatar Jul 14 '25 21:07 noahfalk