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.

Sign up for GitHub

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
Merged

fix: prevent endless recursion loading render tree frames #1132

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8d75dd
fix: prevent endless recursion loading render tree frames
egil Jun 27, 2023
75601f0
Merge branch 'main' into fix-prevent-recursion-loading-render-tree
egil Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/bunit.core/Rendering/TestRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ private void UpdateDisplay(in RenderBatch renderBatch)
{
var id = renderBatch.DisposedComponentIDs.Array[i];

// 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 •
edited
Loading

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.


logger.LogComponentDisposed(id);

if (renderedComponents.TryGetValue(id, out var rc))
Expand Down Expand Up @@ -443,7 +448,8 @@ private void LoadRenderTreeFrames(int componentId, RenderTreeFrameDictionary fra
for (var i = 0; i < frames.Count; i++)
{
ref var frame = ref frames.Array[i];
if (frame.FrameType == RenderTreeFrameType.Component)

if (frame.FrameType == RenderTreeFrameType.Component && !framesCollection.Contains(frame.ComponentId))
{
LoadRenderTreeFrames(frame.ComponentId, framesCollection);
}
Expand Down