WebSocket: observe exceptions in WaitForServerToCloseConnectionAsync

[antonfirsov]

Fixes #80116

Includes a refactor renaming and moving the methods responsible for logging and observing the exceptions. The renaming is for consistency with HttpConnectionBase.LogExceptions in System.Net.Http.

[Copilot]

Pull Request Overview

This PR refactors how exceptions are logged and observed in WebSocket connection close handling, aligning with HttpConnectionBase.LogExceptions for consistency.

• Refactored WaitForServerToCloseConnectionAsync to use LogExceptions instead of Observe.
• Removed duplicated Observe methods in favor of the new LogExceptions implementations.
• Added a regression test to ensure that exceptions during the close handshake are properly observed.

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.cs	Updated WaitForServerToCloseConnectionAsync to log exceptions and refactored logging helper methods.
src/libraries/System.Net.WebSockets/src/System/Net/WebSockets/ManagedWebSocket.KeepAlive.cs	Replaced Observe calls with LogExceptions and removed redundant logging helpers.
src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs	Added a regression test for unobserved exceptions during the close handshake.

Files not reviewed (1)

• src/libraries/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj: Language not supported

Comments suppressed due to low confidence (1)

src/libraries/System.Net.WebSockets.Client/tests/CloseTest.cs:539

• The test registers an event handler for TaskScheduler.UnobservedTaskException but does not unsubscribe it, which could potentially affect other tests. Consider unregistering the event handler after the test completes to ensure isolation.

TaskScheduler.UnobservedTaskException += (obj, args) =>

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

While this is good as a workaround, I cannot not mention that this is a problem of WaitAsync in general (whenever an await finishes due to WaitAsync firing, the task being WaitAsync-ed can inadvertently leak an unobserved exception) -- I wonder if it's already tracked in the Tasks area? We also might need to check if we have such cases of WaitAsync usage anywhere else.

[antonfirsov]

While this is good as a workaround, I cannot not mention that this is a problem of WaitAsync in general (whenever an await finishes due to WaitAsync firing, the task being WaitAsync-ed can inadvertently leak an unobserved exception) -- I wonder if it's already tracked in the Tasks area? We also might need to check if we have such cases of WaitAsync usage anywhere else.

I think we don't want a solution to just swallow the exception with _ = task.Exception, but also to enable logging it with a custom logic. For that we would need an API like WaitAsync(TimeSpan, Action<Task> onTaskFailure).

@stephentoub any thoughts?

[stephentoub]

That already exists in the form of ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted), which this PR is already using.

[CarnaViire]

There seems to be no warning in the API docs for WaitAsync 😔 it might be easy to overlook, and expect the task to somehow "magically" be stopped together with the wait.

However, I see a very nice note for Wait (Wait(CancellationToken), Wait(int, CancellationToken))

(Though this note is a bit hard to notice)

Because it's placed in the very bottom of remarks that in their turn are only after the example 😅

A screenshot to illustrate the relative position:

---

1. I think it might be beneficial to add a similar note to WaitAsync API docs (and move the Wait's note to a more visible place, and maybe expand it to mention the timeout parameter too)
2. I wonder if there is any kind of analyzer check we can add to highlight it? I know there is something that checks cancellation tokens passed in Task.Run

[stephentoub]

I think it might be beneficial to add a similar note to WaitAsync API docs

Sure:
https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Threading.Tasks/Task.xml
:)

[MihaZupan]

Since it's very easy to forget (like we've done) to observe exceptions from the underlying task, would you consider having .WaitAsync itself implicitly suppress unhandled exceptions on the task being awaited when hitting the timeout/cancellation, or would that be too much magic for Task? If that were the case, this change wouldn't be needed.
Having the "correct" usage pattern be to hook up ContinueWith is unfortunate.

[stephentoub]

would you consider having .WaitAsync itself implicitly suppress unhandled exceptions on the task being awaited when hitting the timeout/cancellation, or would that be too much magic for Task?

That assumes a very specific use case, one where you don't care at all about unobserved exceptions from these tasks. As Anton highlighted, we probably want here not to ignore the exception but to log it in some way. And in general the only time unobserved exceptions are visible is when you subscribe to the unobserved exceptions event, in which case you may very well want to know about exceptions that occur after you stop waiting for it.

[antonfirsov]

That already exists in the form of ContinueWith(..., TaskContinuationOptions.OnlyOnFaulted)

Callers still need a lot of ceremonial code around that API. Can be handled by a documentation example instead of a new API I guess.

[CarnaViire]

Was that enough to consistently get an unobserved task exception before the PR change?

[antonfirsov]

Yes, it's failing without the change.

[CarnaViire]

LGTM

[antonfirsov]

CI failures are unrelated.