Skip to content

Enforce circuit termination on unhandled exception #11845

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

Closed
SteveSandersonMS opened this issue Jul 3, 2019 · 0 comments
Closed

Enforce circuit termination on unhandled exception #11845

SteveSandersonMS opened this issue Jul 3, 2019 · 0 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 3, 2019

From #11791

Currently we have many code paths that catch exceptions and then pass them to Renderer.HandleException but otherwise swallow the exception. From there, Renderer.HandleException logs the exception on the server, and sends a (redacted) exception message to the client, which then terminates the connection.

It's not good enough to trust that the client will actually terminate the connection. It might not do, and then the circuit will continue running but in an unknown state.

When fixing this, add tests to show we now enforce server-side circuit termination (but the application process itself continues to run and accept new circuits) in:

  • Exceptions in component instantiation (e.g., in component constructor or IComponent.Configure)
  • Exceptions from IComponent.SetParametersAsync
  • Exceptions from event handlers
    • TBD: We might choose that an unhandled exception from an event handler shouldn't terminate the circuit, but rather just goes back to the client as a JS interop exception. That's kind of the behavior today, except we also have a bug (Renderer.DispatchEventAsync throws null reference exception if event handler throws synchronously #11847).
      • However, it's unclear to me why we'd be loose about event handler exceptions but strict about lifecycle-method exceptions (e.g., in OnInitAsync). We have to be strict about lifecycle-method exceptions because it doesn't make sense to continue using the app when parts of the UI are dead. We would need some kind of "dead component" UI to appear instead of the failed component.
  • Exceptions from OnAfterRenderAsync (called by Renderer.NotifyRenderCompleted)
  • Exceptions that happen during a component's Dispose.
    • But be sure that, if this happens while a circuit itself is being disposed, we still go on to dispose the other components in the circuit and complete the rest of the circuit disposal process (including disposing the DI scope).
@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Jul 3, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview9 milestone Jul 3, 2019
@mkArtakMSFT mkArtakMSFT added the bug This issue describes a behavior which is not expected - a bug. label Jul 3, 2019
@rynowak rynowak self-assigned this Jul 20, 2019
@rynowak rynowak added the Working label Aug 5, 2019
rynowak pushed a commit that referenced this issue Aug 9, 2019
Fixes: #11845

See: #12857 for detailed notes.
rynowak pushed a commit that referenced this issue Aug 11, 2019
Fixes: #11845

See: #12857 for detailed notes.
rynowak pushed a commit that referenced this issue Aug 11, 2019
Fixes: #11845

See: #12857 for detailed notes.
@rynowak rynowak added Done This issue has been fixed and removed Working labels Aug 11, 2019
@rynowak rynowak closed this as completed Aug 11, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

3 participants