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
Changes from 1 commit
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
Loading