Skip to content

Commit 245b6d8

Browse files
egilegil
committed
fix: prevent recursion when determining if rendered component changed
1 parent 3144cf8 commit 245b6d8

File tree

4 files changed

+233
-134
lines changed

4 files changed

+233
-134
lines changed

‎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
@@ -237,80 +237,128 @@ protected override Task UpdateDisplayAsync(in RenderBatch renderBatch)
237237
return Task.CompletedTask;
238238
}
239239

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

266-
return Task.CompletedTask;
267-
}
268-
269-
private void UpdateDisplay(in RenderBatch renderBatch)
270-
{
271-
RenderCount++;
272-
var renderEvent = new RenderEvent(renderBatch, new RenderTreeFrameDictionary());
273-
274-
if (disposed)
254+
foreach (var (key, rc) in renderedComponents)
275255
{
276-
logger.LogRenderCycleActiveAfterDispose();
277-
return;
256+
LoadChangesIntoRenderEvent(rc.ComponentId);
278257
}
279258

280-
// removes disposed components
281-
for (var i = 0; i < renderBatch.DisposedComponentIDs.Count; i++)
282-
{
283-
var id = renderBatch.DisposedComponentIDs.Array[i];
259+
InvokeApplyRenderEvent();
284260

285-
// Add disposed components to the frames collection
286-
// to avoid them being added into the dictionary later during a
287-
// LoadRenderTreeFrames/GetOrLoadRenderTreeFrame call.
288-
renderEvent.Frames.Add(id, default);
261+
return Task.CompletedTask;
262+
263+
void LoadChangesIntoRenderEvent(int componentId)
264+
{
265+
var status = renderEvent.GetStatus(componentId);
266+
if (status.FramesLoaded || status.Disposed)
267+
{
268+
return;
269+
}
289270

290-
logger.LogComponentDisposed(id);
271+
var frames = GetCurrentRenderTreeFrames(componentId);
272+
renderEvent.AddFrames(componentId, frames);
291273

292-
if (renderedComponents.TryGetValue(id, out var rc))
274+
for (var i = 0; i < frames.Count; i++)
293275
{
294-
renderedComponents.Remove(id);
295-
rc.OnRender(renderEvent);
276+
ref var frame = ref frames.Array[i];
277+
if (frame.FrameType == RenderTreeFrameType.Component)
278+
{
279+
var childStatus = renderEvent.GetStatus(frame.ComponentId);
280+
if (childStatus.Disposed)
281+
{
282+
logger.LogDisposedChildInRenderTreeFrame(componentId, frame.ComponentId);
283+
}
284+
else if (!renderEvent.GetStatus(frame.ComponentId).FramesLoaded)
285+
{
286+
LoadChangesIntoRenderEvent(frame.ComponentId);
287+
}
288+
289+
if (childStatus.Rendered || childStatus.Changed || childStatus.Disposed)
290+
{
291+
status.Rendered = status.Rendered || childStatus.Rendered;
292+
status.Changed = status.Changed || childStatus.Changed || childStatus.Disposed;
293+
}
294+
}
296295
}
297296
}
298297

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

304-
rc.OnRender(renderEvent);
328+
private void ApplyRenderEvent(RenderEvent renderEvent)
329+
{
330+
foreach (var (componentId, status) in renderEvent.Statuses)
331+
{
332+
if (status.UpdatesApplied || !renderedComponents.TryGetValue(componentId, out var rc))
333+
{
334+
continue;
335+
}
305336

306-
logger.LogComponentRendered(rc.ComponentId);
337+
if (status.Disposed)
338+
{
339+
renderedComponents.Remove(componentId);
340+
rc.OnRender(renderEvent);
341+
renderEvent.SetUpdatedApplied(componentId);
342+
logger.LogComponentDisposed(componentId);
343+
continue;
344+
}
307345

308-
// RC can replace the instance of the component it is bound
309-
// to while processing the update event.
310-
if (key != rc.ComponentId)
346+
if (status.UpdateNeeded)
311347
{
312-
renderedComponents.Remove(key);
313-
renderedComponents.Add(rc.ComponentId, rc);
348+
rc.OnRender(renderEvent);
349+
renderEvent.SetUpdatedApplied(componentId);
350+
351+
// RC can replace the instance of the component it is bound
352+
// to while processing the update event, e.g. during the
353+
// initial render of a component.
354+
if (componentId != rc.ComponentId)
355+
{
356+
renderedComponents.Remove(componentId);
357+
renderedComponents.Add(rc.ComponentId, rc);
358+
renderEvent.SetUpdatedApplied(rc.ComponentId);
359+
}
360+
361+
logger.LogComponentRendered(rc.ComponentId);
314362
}
315363
}
316364
}
@@ -451,7 +499,7 @@ private void LoadRenderTreeFrames(int componentId, RenderTreeFrameDictionary fra
451499
for (var i = 0; i < frames.Count; i++)
452500
{
453501
ref var frame = ref frames.Array[i];
454-
502+
455503
if (frame.FrameType == RenderTreeFrameType.Component && !framesCollection.Contains(frame.ComponentId))
456504
{
457505
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)