- Notifications
You must be signed in to change notification settings - Fork10.4k
PreventObjectDisposedException 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| catch (ObjectDisposedException ex) | ||
| { | ||
| // Expected when service provider is disposed during circuit cleanup e.g. by forced GC |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Line 38 in9f85d05
| GC.Collect(); |
It looks like after that operation (that cleaned DI services), we are trying to do:
| varpersistenceManager=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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
The logging added in the previous commit is not that helpful - it's mostly duplication of current call stack printed in the log. I'll create an issue and we will observe the CI. We should come back to this PR to investigate f we are not calling dispose multiple times and why the exception happens. |
Issue similar to#62554.
log
Description
PeristanceManager.PauseCircuitAsynctries to resolve theComponentStatePersistenceManagerservice from an already disposed service provider during circuit cleanup (triggered by garbage collection), causing anObjectDisposedExceptionto be thrown.aspnetcore/src/Components/Server/src/Circuits/CircuitPersistenceManager.cs
Line 25 inea282bf
Theoretically we are subscribed to
circuitHost.UnhandledExceptionbut it did not get triggered in this situation. The fix is to observe the exception. I choseCircuitRegistryfor it because it hasCircuitHost_UnhandledExceptionmethod to handle that gracefully.This failure was obsered once on CI, see the log above. Running that test locally on repeat did not reveal any hits after 1k runs.