Dispose components on client disconnects #6693
Conversation
| } | ||
| catch | ||
| { | ||
| // Ignore exceptions thrown by components when they're being disposed. |
There was a problem hiding this comment.
We should log them somehow though, shouldn't we? If someone's disposal logic is failing they will need a way to know about it. This isn't the entire app shutting down, it's just a user closing a browser tab.
Instead of swallowing them, could we capture them all in an AggregateException and then throw it the the end of the upstream Dispose method?
There was a problem hiding this comment.
Actually, could you clarify why this code needs to be changed? It doesn't look like it's involved in the circuit teardown process anyway.
There was a problem hiding this comment.
I went by the comment that suggested something need to be done about exceptions. We can handle this separately
|
|
||
| namespace Microsoft.AspNetCore.Components.Server.Circuits | ||
| { | ||
| public class CircuirHostTest |
| } | ||
| catch | ||
| { | ||
| // Ignore failures in components throwing |
There was a problem hiding this comment.
Although I agree that we shouldn't stop the foreach if there's an exception, I'm not convinced we should be swallowing the exceptions altogether. Do you think it would be reasonable to capture them all in an AggregateException and then throw it at the end of this method?
| @@ -1,14 +1,13 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
| using Microsoft.AspNetCore.Components.Rendering; | |||
| using Microsoft.AspNetCore.Components.RenderTree; | |||
| using Microsoft.AspNetCore.Components.Test.Helpers; | |||
There was a problem hiding this comment.
Should we create an editor config so these things stop changing? Our eng guidelines says system first.
There was a problem hiding this comment.
Copied over the .editorconfig from MVC. I'll follow up with engg to commonize all the .editorconfigs
| @@ -1139,6 +1138,21 @@ public async Task CanTriggerEventHandlerDisposedInEarlierPendingBatch() | |||
| Assert.Equal(2, numEventsFired); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void DisposingRenderer_DisposesComponents() | |||
There was a problem hiding this comment.
It would be great to also have a unit test to show we dispose non-root components too. The first "arrange" section of the CanRenderNestedComponents test shows a convenient way to set up that scenario.
| @@ -1,14 +1,13 @@ | |||
| // Copyright (c) .NET Foundation. All rights reserved. | |||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
| using Microsoft.AspNetCore.Components.Rendering; | |||
| using Microsoft.AspNetCore.Components.RenderTree; | |||
| using Microsoft.AspNetCore.Components.Test.Helpers; | |||
There was a problem hiding this comment.
Copied over the .editorconfig from MVC. I'll follow up with engg to commonize all the .editorconfigs
| if (exceptions != null) | ||
| { | ||
| throw new AggregateException(exceptions); | ||
| } |
There was a problem hiding this comment.
Do we have an issue somewhere to clean this up? Throwing inside dispose isn't a nice thing to do, especially when you have inheritance involved. There are a bunch of places in this PR that will leak stuff on failed dispose 😆
I realize that you didn't make it worse in this PR, but it is gross so I want to make sure it's on someone's plate to fix.
There was a problem hiding this comment.
Added it to this issue - #5476
| using Moq; | ||
| using System; | ||
| using System.Threading; | ||
| using Xunit; |
pranavkm
force-pushed
the
prkrishn/circuits
branch
from
8d05830 to
7c25574
Compare
pranavkm
force-pushed
the
prkrishn/circuits
branch
from
7c25574 to
7cdf823
Compare
Fixes #4047