Skip to content

Commit 88655c1

Browse files
egilegil
committed
fix: prevent recursion when determining if rendered component changed
1 parent b6574ac commit 88655c1

4 files changed

Lines changed: 233 additions & 134 deletions

File tree

‎src/bunit.core/Rendering/RenderEvent.cs‎

Lines changed: 46 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,15 @@ namespace Bunit.Rendering;
55
/// </summary>
66
public sealed class RenderEvent
77
{
8-
private readonly RenderBatch renderBatch;
8+
private readonly Dictionary<int, Status> statuses = new();
9+
10+
internal IReadOnlyDictionary<int, Status> Statuses => statuses;
911

1012
/// <summary>
1113
/// Gets a collection of <see cref="ArrayRange{RenderTreeFrame}"/>, accessible via the ID
1214
/// of the component they are created by.
1315
/// </summary>
14-
public RenderTreeFrameDictionary Frames { get; }
15-
16-
/// <summary>
17-
/// Initializes a new instance of the <see cref="RenderEvent"/> class.
18-
/// </summary>
19-
/// <param name="renderBatch">The <see cref="RenderBatch"/> update from the render event.</param>
20-
/// <param name="frames">The <see cref="RenderTreeFrameDictionary"/> from the current render.</param>
21-
internal RenderEvent(RenderBatch renderBatch, RenderTreeFrameDictionary frames)
22-
{
23-
this.renderBatch = renderBatch;
24-
Frames = frames;
25-
}
16+
public RenderTreeFrameDictionary Frames { get; } = new();
2617

2718
/// <summary>
2819
/// Gets the render status for a <paramref name="renderedComponent"/>.
@@ -34,86 +25,62 @@ internal RenderEvent(RenderBatch renderBatch, RenderTreeFrameDictionary frames)
3425
if (renderedComponent is null)
3526
throw new ArgumentNullException(nameof(renderedComponent));
3627

37-
var result = (Rendered: false, Changed: false, Disposed: false);
28+
return statuses.TryGetValue(renderedComponent.ComponentId, out var status)
29+
? (status.Rendered, status.Changed, status.Disposed)
30+
: (Rendered: false, Changed: false, Disposed: false);
31+
}
3832

39-
if (DidComponentDispose(renderedComponent))
40-
{
41-
result.Disposed = true;
42-
}
43-
else
33+
internal Status GetStatus(int componentId)
34+
{
35+
if (!statuses.TryGetValue(componentId, out var status))
4436
{
45-
(result.Rendered, result.Changed) = GetRenderAndChangeStatus(renderedComponent);
37+
status = new();
38+
statuses[componentId] = status;
4639
}
40+
return status;
41+
}
4742

48-
return result;
43+
internal void SetDisposed(int componentId)
44+
{
45+
GetStatus(componentId).Disposed = true;
4946
}
5047

51-
private bool DidComponentDispose(IRenderedFragmentBase renderedComponent)
48+
internal void SetUpdated(int componentId, bool hasChanges)
5249
{
53-
for (var i = 0; i < renderBatch.DisposedComponentIDs.Count; i++)
54-
{
55-
if (renderBatch.DisposedComponentIDs.Array[i].Equals(renderedComponent.ComponentId))
56-
{
57-
return true;
58-
}
59-
}
50+
var status = GetStatus(componentId);
51+
status.Rendered = true;
52+
status.Changed = hasChanges;
53+
}
6054

61-
return false;
55+
internal void SetFramesLoaded(int componentId)
56+
{
57+
GetStatus(componentId).FramesLoaded = true;
6258
}
6359

64-
/// <summary>
65-
/// This method determines if the <paramref name="renderedComponent"/> or any of the
66-
/// components underneath it in the render tree rendered and whether they they changed
67-
/// their render tree during render.
68-
///
69-
/// It does this by getting the status from the <paramref name="renderedComponent"/>,
70-
/// then from all its children, using a recursive pattern, where the internal methods
71-
/// GetStatus and GetStatusFromChildren call each other until there are no more children,
72-
/// or both a render and a change is found.
73-
/// </summary>
74-
private (bool Rendered, bool HasChanges) GetRenderAndChangeStatus(IRenderedFragmentBase renderedComponent)
60+
internal void SetUpdatedApplied(int componentId)
7561
{
76-
var result = (Rendered: false, HasChanges: false);
62+
GetStatus(componentId).UpdatesApplied = true;
63+
}
7764

78-
GetStatus(renderedComponent.ComponentId);
65+
internal void AddFrames(int componentId, ArrayRange<RenderTreeFrame> frames)
66+
{
67+
Frames.Add(componentId, frames);
68+
GetStatus(componentId).FramesLoaded = true;
69+
}
7970

80-
return result;
71+
internal record class Status
72+
{
73+
public bool Rendered { get; set; }
8174

82-
void GetStatus(int componentId)
83-
{
84-
for (var i = 0; i < renderBatch.UpdatedComponents.Count; i++)
85-
{
86-
ref var update = ref renderBatch.UpdatedComponents.Array[i];
87-
if (update.ComponentId == componentId)
88-
{
89-
result.Rendered = true;
90-
result.HasChanges = update.Edits.Count > 0;
91-
break;
92-
}
93-
}
94-
95-
if (!result.HasChanges)
96-
{
97-
GetStatusFromChildren(componentId);
98-
}
99-
}
75+
public bool Changed { get; set; }
10076

101-
void GetStatusFromChildren(int componentId)
102-
{
103-
var frames = Frames[componentId];
104-
for (var i = 0; i < frames.Count; i++)
105-
{
106-
ref var frame = ref frames.Array[i];
107-
if (frame.FrameType == RenderTreeFrameType.Component)
108-
{
109-
GetStatus(frame.ComponentId);
110-
111-
if (result.HasChanges)
112-
{
113-
break;
114-
}
115-
}
116-
}
117-
}
77+
public bool Disposed { get; set; }
78+
79+
public bool UpdatesApplied { get; set; }
80+
81+
public bool FramesLoaded { get; set; }
82+
83+
public bool UpdateNeeded => Rendered || Changed;
11884
}
11985
}
86+

‎src/bunit.core/Rendering/TestRenderer.cs‎

Lines changed: 103 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -247,80 +247,128 @@ protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
247247
return Task.CompletedTask;
248248
}
249249

250-
if (usersSyncContext is not null && usersSyncContext != SynchronizationContext.Current)
250+
var renderEvent = new RenderEvent();
251+
252+
for (var i = 0; i < renderBatch.DisposedComponentIDs.Count; i++)
251253
{
252-
// The users' sync context, typically one established by
253-
// xUnit or another testing framework is used to update any
254-
// rendered fragments/dom trees and trigger WaitForX handlers.
255-
// This ensures that changes to DOM observed inside a WaitForX handler
256-
// will also be visible outside a WaitForX handler, since
257-
// they will be running in the same sync context. The theory is that
258-
// this should mitigate the issues where Blazor's dispatcher/thread is used
259-
// to verify an assertion inside a WaitForX handler, and another thread is
260-
// used again to access the DOM/repeat the assertion, where the change
261-
// may not be visible yet (another theory about why that may happen is different
262-
// CPU cache updates not happening immediately).
263-
//
264-
// There is no guarantee a caller/test framework has set a sync context.
265-
usersSyncContext.Send(static (state) =>
266-
{
267-
var (renderBatch, renderer) = ((RenderBatch, TestRenderer))state!;
268-
renderer.UpdateDisplay(renderBatch);
269-
}, (renderBatch, this));
254+
var id = renderBatch.DisposedComponentIDs.Array[i];
255+
renderEvent.SetDisposed(id);
270256
}
271-
else
257+
258+
for (int i = 0; i < renderBatch.UpdatedComponents.Count; i++)
272259
{
273-
UpdateDisplay(renderBatch);
260+
ref var update = ref renderBatch.UpdatedComponents.Array[i];
261+
renderEvent.SetUpdated(update.ComponentId, update.Edits.Count > 0);
274262
}
275263

276-
return Task.CompletedTask;
277-
}
278-
279-
private void UpdateDisplay(in RenderBatch renderBatch)
280-
{
281-
RenderCount++;
282-
var renderEvent = new RenderEvent(renderBatch, new RenderTreeFrameDictionary());
283-
284-
if (disposed)
264+
foreach (var (key, rc) in renderedComponents)
285265
{
286-
logger.LogRenderCycleActiveAfterDispose();
287-
return;
266+
LoadChangesIntoRenderEvent(rc.ComponentId);
288267
}
289268

290-
// removes disposed components
291-
for (var i = 0; i < renderBatch.DisposedComponentIDs.Count; i++)
292-
{
293-
var id = renderBatch.DisposedComponentIDs.Array[i];
269+
InvokeApplyRenderEvent();
294270

295-
// Add disposed components to the frames collection
296-
// to avoid them being added into the dictionary later during a
297-
// LoadRenderTreeFrames/GetOrLoadRenderTreeFrame call.
298-
renderEvent.Frames.Add(id, default);
271+
return Task.CompletedTask;
272+
273+
void LoadChangesIntoRenderEvent(int componentId)
274+
{
275+
var status = renderEvent.GetStatus(componentId);
276+
if (status.FramesLoaded || status.Disposed)
277+
{
278+
return;
279+
}
299280

300-
logger.LogComponentDisposed(id);
281+
var frames = GetCurrentRenderTreeFrames(componentId);
282+
renderEvent.AddFrames(componentId, frames);
301283

302-
if (renderedComponents.TryGetValue(id, out var rc))
284+
for (var i = 0; i < frames.Count; i++)
303285
{
304-
renderedComponents.Remove(id);
305-
rc.OnRender(renderEvent);
286+
ref var frame = ref frames.Array[i];
287+
if (frame.FrameType == RenderTreeFrameType.Component)
288+
{
289+
var childStatus = renderEvent.GetStatus(frame.ComponentId);
290+
if (childStatus.Disposed)
291+
{
292+
logger.LogDisposedChildInRenderTreeFrame(componentId, frame.ComponentId);
293+
}
294+
else if (!renderEvent.GetStatus(frame.ComponentId).FramesLoaded)
295+
{
296+
LoadChangesIntoRenderEvent(frame.ComponentId);
297+
}
298+
299+
if (childStatus.Rendered || childStatus.Changed || childStatus.Disposed)
300+
{
301+
status.Rendered = status.Rendered || childStatus.Rendered;
302+
status.Changed = status.Changed || childStatus.Changed || childStatus.Disposed;
303+
}
304+
}
306305
}
307306
}
308307

309-
// notify each rendered component about the render
310-
foreach (var (key, rc) in renderedComponents.ToArray())
308+
void InvokeApplyRenderEvent()
311309
{
312-
LoadRenderTreeFrames(rc.ComponentId, renderEvent.Frames);
310+
if (usersSyncContext is not null && usersSyncContext != SynchronizationContext.Current)
311+
{
312+
// The users' sync context, typically one established by
313+
// xUnit or another testing framework is used to update any
314+
// rendered fragments/dom trees and trigger WaitForX handlers.
315+
// This ensures that changes to DOM observed inside a WaitForX handler
316+
// will also be visible outside a WaitForX handler, since
317+
// they will be running in the same sync context. The theory is that
318+
// this should mitigate the issues where Blazor's dispatcher/thread is used
319+
// to verify an assertion inside a WaitForX handler, and another thread is
320+
// used again to access the DOM/repeat the assertion, where the change
321+
// may not be visible yet (another theory about why that may happen is different
322+
// CPU cache updates not happening immediately).
323+
//
324+
// There is no guarantee a caller/test framework has set a sync context.
325+
usersSyncContext.Send(static (state) =>
326+
{
327+
var (renderEvent, renderer) = ((RenderEvent, TestRenderer))state!;
328+
renderer.ApplyRenderEvent(renderEvent);
329+
}, (renderEvent, this));
330+
}
331+
else
332+
{
333+
ApplyRenderEvent(renderEvent);
334+
}
335+
}
336+
}
313337

314-
rc.OnRender(renderEvent);
338+
private void ApplyRenderEvent(RenderEvent renderEvent)
339+
{
340+
foreach (var (componentId, status) in renderEvent.Statuses)
341+
{
342+
if (status.UpdatesApplied || !renderedComponents.TryGetValue(componentId, out var rc))
343+
{
344+
continue;
345+
}
315346

316-
logger.LogComponentRendered(rc.ComponentId);
347+
if (status.Disposed)
348+
{
349+
renderedComponents.Remove(componentId);
350+
rc.OnRender(renderEvent);
351+
renderEvent.SetUpdatedApplied(componentId);
352+
logger.LogComponentDisposed(componentId);
353+
continue;
354+
}
317355

318-
// RC can replace the instance of the component it is bound
319-
// to while processing the update event.
320-
if (key != rc.ComponentId)
356+
if (status.UpdateNeeded)
321357
{
322-
renderedComponents.Remove(key);
323-
renderedComponents.Add(rc.ComponentId, rc);
358+
rc.OnRender(renderEvent);
359+
renderEvent.SetUpdatedApplied(componentId);
360+
361+
// RC can replace the instance of the component it is bound
362+
// to while processing the update event, e.g. during the
363+
// initial render of a component.
364+
if (componentId != rc.ComponentId)
365+
{
366+
renderedComponents.Remove(componentId);
367+
renderedComponents.Add(rc.ComponentId, rc);
368+
renderEvent.SetUpdatedApplied(rc.ComponentId);
369+
}
370+
371+
logger.LogComponentRendered(rc.ComponentId);
324372
}
325373
}
326374
}
@@ -461,7 +509,7 @@ private void LoadRenderTreeFrames(int componentId, RenderTreeFrameDictionary fra
461509
for (var i = 0; i < frames.Count; i++)
462510
{
463511
ref var frame = ref frames.Array[i];
464-
512+
465513
if (frame.FrameType == RenderTreeFrameType.Component && !framesCollection.Contains(frame.ComponentId))
466514
{
467515
LoadRenderTreeFrames(frame.ComponentId, framesCollection);

‎src/bunit.core/Rendering/TestRendererLoggerExtensions.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ internal static class TestRendererLoggerExtensions
1919
private static readonly Action<ILogger, Exception?> ChangedComponentsMarkupUpdated
2020
= LoggerMessage.Define(LogLevel.Debug, new EventId(13, "UpdateDisplayAsync"), "Finished updating the markup of changed components.");
2121

22+
private static readonly Action<ILogger, int, int, Exception?> DisposedChildInRenderTreeFrame
23+
= LoggerMessage.Define<int, int>(LogLevel.Warning, new EventId(14, "UpdateDisplayAsync"), "A parent components {ParentComponentId} has a disposed component {ComponentId} as its child.");
24+
2225
private static readonly Action<ILogger, Exception?> AsyncInitialRender
2326
= LoggerMessage.Define(LogLevel.Debug, new EventId(20, "Render"), "The initial render task did not complete immediately.");
2427

@@ -102,4 +105,12 @@ internal static void LogRenderCycleActiveAfterDispose(this ILogger<TestRenderer>
102105
RenderCycleActiveAfterDispose(logger, null);
103106
}
104107
}
108+
109+
internal static void LogDisposedChildInRenderTreeFrame(this ILogger<TestRenderer> logger, int parentComponentId, int componentId)
110+
{
111+
if (logger.IsEnabled(LogLevel.Warning))
112+
{
113+
DisposedChildInRenderTreeFrame(logger, parentComponentId, componentId, null);
114+
}
115+
}
105116
}

0 commit comments

Comments
 (0)