Ensure DI scope is disposed #11894
Ensure DI scope is disposed #11894
Conversation
|
|
||
| namespace Microsoft.AspNetCore.Components.Server.Circuits | ||
| { | ||
| internal class CircuitPrerenderer : IComponentPrerenderer | ||
| { | ||
| private static object CircuitHostKey = new object(); | ||
| private static object NavigationStatusKey = new object(); | ||
| private static object CancellationStatusKey = new object(); |
There was a problem hiding this comment.
Most of the noise in the diff here is just because I renamed the concept of NavigationStatus to CancellationStatus.
The code was never really interested in the concept of "did we navigate or not" - it was only interested in "should the circuit continue to exist", so this is a more precise name. It's relevant to change this now because there's now a second case where the circuit should not continue to exist (i.e., some component threw during prerendering, so we gave up on prerendering and released the DI scope immediately).
There was a problem hiding this comment.
(i.e., some component threw during prerendering, so we gave up on prerendering and released the DI scope immediately).
I'm not sure what the E2E behavior is, but when multiple components get prerendered at the same time they share the same circuit, so isn't it bad to prevent other prerendered components from working? (limiting the error to the current prerendered component)
There was a problem hiding this comment.
When a component throws an unhandled exception in prerendering, we return a 500 error to the browser, not a partially-complete HTML response.
There was a problem hiding this comment.
I still have some doubts about this because people could write a try catch for these scenarios to keep other components in the page working and we are taking away that option, I'm not sure if that is something we want to consider.
(For example, don't dispose the circuit if some other component rendered successfully and only do the cleanup for the circuit on response started if we got a 500 out)
There was a problem hiding this comment.
That's a good question to raise, but it makes me think maybe we shouldn't be putting all prerendered components into the same circuit, or should give some other way to control how they are grouped into circuits so those groups can fail independently.
This is because in non-prerendering cases, we don't allow circuits/renderers/etc to continue to exist and be used once they have got into an undefined state through an unhandled exception during rendering or lifecycle methods. So in that sense it's good that we prevent it for prerendering too, and don't allow people to try/catch and then continue using that circuit state.
There was a problem hiding this comment.
I think that's a fair answer, given that we are moving to a single circuit per connection model (independent of whether its prerendered or not)
I think that makes sense and leaves things in a more manageable state, so I'm ok with it.
There was a problem hiding this comment.
I don't think we should work hard to make life better when something is crashing. We should make sure that it fails hard, if users have multiple components on a page and they want to degrade gracefully they need to do it in their own code.
So I agree with the conclusion here.
| { | ||
| // Avoid creating a circuit host if other component earlier in the pipeline already triggered | ||
| // a navigation request. Instead rendre nothing | ||
| // cancelation (e.g., by navigating or throwing). Instead render nothing. |
There was a problem hiding this comment.
nit, mixture of cancellation and cancelation
There was a problem hiding this comment.
Good spot. I'll fix the comment typo in an unrelated PR just so I don't have to wait for CI checks here again.
Fixes #11848. Addresses the two cases from that issue: