Skip to content

Prevent ObjectDisposedException from becoming unobserved during circuit cleanup #62662

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions src/Components/Server/src/Circuits/CircuitRegistry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,20 @@ private Task PauseAndDisposeCircuitEntry(DisconnectedCircuitEntry entry)

private async Task PauseAndDisposeCircuitHost(CircuitHost circuitHost, bool saveStateToClient)
{
await _circuitPersistenceManager.PauseCircuitAsync(circuitHost, saveStateToClient);
circuitHost.UnhandledException -= CircuitHost_UnhandledException;
await circuitHost.DisposeAsync();
try
{
await _circuitPersistenceManager.PauseCircuitAsync(circuitHost, saveStateToClient);
}
catch (ObjectDisposedException ex)
{
// Expected when service provider is disposed during circuit cleanup e.g. by forced GC
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this bit super well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test that was failing is forcing GC.


It looks like after that operation (that cleaned DI services), we are trying to do:
var persistenceManager = circuit.Services.GetRequiredService<ComponentStatePersistenceManager>();

Which is throwing. I'm not sure how else we can get to such situations, if we don't force GC - which is not a common pattern. However, I don't see any harm letting the exception be observed.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to track the root cause of the objectdisposedexception, as it might be pointing out a real issue. If you are able to reproduce it, can you capture the callstack for the object disposed exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot reproduce it. I really tried, 1k runs locally and no failure

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are able to reproduce it, can you capture the callstack for the object disposed exception

We could add more logging of inner exception in the test and count on it to fail again on CI. However, we don't know if it will be on a PR that someone will report the failure. Or will they just hit rerun and be happy that it passed the next time.

CircuitHost_UnhandledException(circuitHost, new UnhandledExceptionEventArgs(ex, isTerminating: true));
}
finally
{
circuitHost.UnhandledException -= CircuitHost_UnhandledException;
await circuitHost.DisposeAsync();
}
}

internal async Task PauseCircuitAsync(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,39 @@
{
<h2>Unobserved Exceptions (for debugging):</h2>
<ul>
@foreach (var ex in _unobservedExceptions)
@foreach (var exceptionDetail in _unobservedExceptions)
{
<li>@ex.ToString()</li>
<li>
<strong>Observed at:</strong> @exceptionDetail.ObservedAt.ToString("yyyy-MM-dd HH:mm:ss.fff") UTC<br/>
<strong>Thread ID:</strong> @exceptionDetail.ObservedThreadId<br/>
<strong>From Finalizer Thread:</strong> @exceptionDetail.IsFromFinalizerThread<br/>
<strong>Exception:</strong> @exceptionDetail.Exception.ToString()<br/>
<details>
<summary>Detailed Exception Information</summary>
<pre>@exceptionDetail.DetailedExceptionInfo</pre>
</details>
<details>
<summary>Call Stack When Observed</summary>
<pre>@exceptionDetail.ObservedCallStack</pre>
</details>
</li>
}
</ul>
}
}

@code {
private bool _shouldStopRedirecting;
private IReadOnlyCollection<Exception> _unobservedExceptions = Array.Empty<Exception>();
private IReadOnlyCollection<UnobservedExceptionDetails> _unobservedExceptions = Array.Empty<UnobservedExceptionDetails>();

protected override async Task OnInitializedAsync()
{
int visits = Observer.GetCircularRedirectCount();
if (visits == 0)
{
// make sure we start with clean logs
Observer.Clear();
}
int visits = Observer.GetCircularRedirectCount();
if (visits == 0)
{
// make sure we start with clean logs
Observer.Clear();
}

// Force GC collection to trigger finalizers - this is what causes the issue
GC.Collect();
Expand All @@ -47,7 +60,7 @@
else
{
_shouldStopRedirecting = true;
_unobservedExceptions = Observer.GetExceptions();
_unobservedExceptions = Observer.GetExceptionDetails();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,134 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Concurrent;
using System.Diagnostics;
using System.Globalization;
using System.Text;
using System.Threading;
using System.Linq;

namespace TestServer;

/// <summary>
/// Represents detailed information about an unobserved task exception, including the original call stack.
/// </summary>
public class UnobservedExceptionDetails
{
/// <summary>
/// The original exception that was unobserved.
/// </summary>
public Exception Exception { get; init; }

/// <summary>
/// The timestamp when the exception was observed.
/// </summary>
public DateTime ObservedAt { get; init; }

/// <summary>
/// The current call stack when the exception was observed (may show finalizer thread).
/// </summary>
public string ObservedCallStack { get; init; }

/// <summary>
/// Detailed breakdown of inner exceptions and their stack traces.
/// </summary>
public string DetailedExceptionInfo { get; init; }

/// <summary>
/// The managed thread ID where the exception was observed.
/// </summary>
public int ObservedThreadId { get; init; }

/// <summary>
/// Whether this exception was observed on the finalizer thread.
/// </summary>
public bool IsFromFinalizerThread { get; init; }

public UnobservedExceptionDetails(Exception exception)
{
Exception = exception;
ObservedAt = DateTime.UtcNow;
ObservedCallStack = Environment.StackTrace;
DetailedExceptionInfo = BuildDetailedExceptionInfo(exception);
ObservedThreadId = Environment.CurrentManagedThreadId;
IsFromFinalizerThread = Thread.CurrentThread.IsThreadPoolThread && Thread.CurrentThread.IsBackground;
}

private static string BuildDetailedExceptionInfo(Exception exception)
{
var sb = new StringBuilder();
var currentException = exception;
var depth = 0;

while (currentException is not null)
{
var indent = new string(' ', depth * 2);
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}Exception Type: {currentException.GetType().FullName}");
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}Message: {currentException.Message}");

if (currentException.Data.Count > 0)
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}Data:");
foreach (var key in currentException.Data.Keys)
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent} {key}: {currentException.Data[key]}");
}
}

if (!string.IsNullOrEmpty(currentException.StackTrace))
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}Stack Trace:");
sb.AppendLine(currentException.StackTrace);
}

// Handle AggregateException specially to extract all inner exceptions
if (currentException is AggregateException aggregateException)
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}Aggregate Exception contains {aggregateException.InnerExceptions.Count} inner exceptions:");
for (int i = 0; i < aggregateException.InnerExceptions.Count; i++)
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent} Inner Exception {i + 1}:");
sb.AppendLine(BuildDetailedExceptionInfo(aggregateException.InnerExceptions[i]));
}
break; // Don't process InnerException for AggregateException as we've handled all inner exceptions
}

currentException = currentException.InnerException;
depth++;

if (currentException is not null)
{
sb.AppendLine(CultureInfo.InvariantCulture, $"{indent}--- Inner Exception ---");
}
}

return sb.ToString();
}
}

public class UnobservedTaskExceptionObserver
{
private readonly ConcurrentQueue<Exception> _exceptions = new();
private readonly ConcurrentQueue<UnobservedExceptionDetails> _exceptions = new();
private int _circularRedirectCount;

public void OnUnobservedTaskException(object sender, UnobservedTaskExceptionEventArgs e)
{
_exceptions.Enqueue(e.Exception);
var details = new UnobservedExceptionDetails(e.Exception);
_exceptions.Enqueue(details);
e.SetObserved(); // Mark as observed to prevent the process from crashing during tests
}

public bool HasExceptions => !_exceptions.IsEmpty;

public IReadOnlyCollection<Exception> GetExceptions() => _exceptions.ToArray();
/// <summary>
/// Gets the detailed exception information including original call stacks.
/// </summary>
public IReadOnlyCollection<UnobservedExceptionDetails> GetExceptionDetails() => _exceptions.ToArray();

/// <summary>
/// Gets the raw exceptions for backward compatibility.
/// </summary>
public IReadOnlyCollection<Exception> GetExceptions() => _exceptions.ToArray().Select(d => d.Exception).ToList();

public void Clear()
{
Expand Down
Loading