Skip to content

fix: prevent endless recursion loading render tree frames #1132

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

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

egil
Copy link
Member

@egil egil commented Jun 27, 2023

Look at the logs that @groogiam has shared with me, I see a bunch of renders happening after the TestRenderer has been disposed, i.e. LogRenderCycleActiveAfterDispose is being triggered.

However, that does not explain the stack overflow. Thus, the following code should prevent loading render tree frames from being loaded in an endless loop, perhaps because were are attempting load render tree frames for disposed components.

fixes #1064

PR meta checklist

  • Pull request is targeted at main branch for code
    or targeted at stable branch for documentation that is live on bunit.dev.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Code PR specific checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@egil egil requested a review from linkdotnet June 27, 2023 23:54
// Add disposed components to the frames collection
// to avoid them being added into the dictionary later during a
// LoadRenderTreeFrames/GetOrLoadRenderTreeFrame call.
renderEvent.Frames.Add(id, default);
Copy link
Collaborator

@linkdotnet linkdotnet Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we remove the component on line 290 - it would be also nice to understand if we run into an else case:

if (renderedComponents.TryGetValue(id, out var rc))
{
...
}
else
{
  throw new UppsieException("Wuppsie, that should not happen!");
}

Or we just look the stuff there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect much of a change if we hold a second list of disposed objects and one where we remove disposed objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand.

My assumption is that even though a component is disposed, it may still linger in the current render tree and be picked up when we walk through that.

Yes, we remove the component in line 290, but that is just our internal tracking of that component.

But yes, it would be interesting to see if disposed components are still lingering in the render tree.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duhhh - yeah that is on me. I missed something there. Anyway I checked the code a bit deeper.
Wouldn't be easier to do the following:

if (frame.FrameType == RenderTreeFrameType.Component && renderedComponents.Contains(frame.ComponentId))

We don't need a new list for an information we already have.

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 want to publish a preview package based on this code to have @groogiam try out the preview locally since he cannot share the code with me so I can experiment locally myself.

If that works, Ill try to clean things up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be easier to do the following:

if (frame.FrameType == RenderTreeFrameType.Component && renderedComponents.Contains(frame.ComponentId))

Unfortunately not, since the renderedComponents collection only has the rendered components we have returned to the user, not all components.

@egil egil merged commit 4e439e9 into main Jun 28, 2023
@egil egil deleted the fix-prevent-recursion-loading-render-tree branch June 28, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.19.14 dotnet test crashes when running multiple tests
2 participants