Skip to content
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

Renderer support for removing root components or updating parameters on them #34232

Merged
merged 6 commits into from
Jul 13, 2021
Merged

Renderer support for removing root components or updating parameters on them #34232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
719597a
Support removal of root components
SteveSandersonMS Jul 9, 2021
41d9ddb
Support supplying new parameters to root components, even while not q…
SteveSandersonMS Jul 9, 2021
d573961
Tidyings
SteveSandersonMS Jul 12, 2021
09bba1e
Fix unit tests, now we better validate use of sync context
SteveSandersonMS Jul 12, 2021
8d71c1e
Fix some unrelated unit tests while I'm here
SteveSandersonMS Jul 12, 2021
a92a11e
Update after rebase
SteveSandersonMS Jul 12, 2021
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
1 change: 1 addition & 0 deletions src/Components/Components/src/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Microsoft.AspNetCore.Components.NavigationOptions.NavigationOptions() -> void
Microsoft.AspNetCore.Components.NavigationOptions.ReplaceHistoryEntry.get -> bool
Microsoft.AspNetCore.Components.NavigationOptions.ReplaceHistoryEntry.init -> void
Microsoft.AspNetCore.Components.RenderHandle.IsRenderingOnMetadataUpdate.get -> bool
Microsoft.AspNetCore.Components.RenderTree.Renderer.RemoveRootComponent(int componentId) -> void
Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder.Dispose() -> void
Microsoft.AspNetCore.Components.Routing.Router.PreferExactMatches.get -> bool
Microsoft.AspNetCore.Components.Routing.Router.PreferExactMatches.set -> void
Expand Down
144 changes: 100 additions & 44 deletions src/Components/Components/src/RenderTree/Renderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable
private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>();
private readonly ILogger<Renderer> _logger;
private readonly ComponentFactory _componentFactory;
private List<(ComponentState, ParameterView)>? _rootComponents;
private Dictionary<int, ParameterView>? _rootComponentsLatestParameters;
private Task? _ongoingQuiescenceTask;

private int _nextComponentId;
private bool _isBatchInProgress;
Expand Down Expand Up @@ -134,17 +135,18 @@ private async void RenderRootComponentsOnHotReload()

await Dispatcher.InvokeAsync(() =>
{
if (_rootComponents is null)
if (_rootComponentsLatestParameters is null)
{
return;
}

IsRenderingOnMetadataUpdate = true;
try
{
foreach (var (componentState, initialParameters) in _rootComponents)
foreach (var (componentId, parameters) in _rootComponentsLatestParameters)
{
componentState.SetDirectParameters(initialParameters);
var componentState = GetRequiredComponentState(componentId);
componentState.SetDirectParameters(parameters);
}
}
finally
Expand Down Expand Up @@ -199,53 +201,71 @@ protected Task RenderRootComponentAsync(int componentId)
}

/// <summary>
/// Performs the first render for a root component, waiting for this component and all
/// children components to finish rendering in case there is any asynchronous work being
/// done by any of the components. After this, the root component
/// makes its own decisions about when to re-render, so there is no need to call
/// this more than once.
/// Supplies parameters for a root component, normally causing it to render. This can be
/// used to trigger the first render of a root component, or to update its parameters and
/// trigger a subsequent render. Note that components may also make their own decisions about
/// when to re-render, and may re-render at any time.
///
/// The returned <see cref="Task"/> waits for this component and all descendant components to
/// finish rendering in case there is any asynchronous work being done by any of them.
/// </summary>
/// <param name="componentId">The ID returned by <see cref="AssignRootComponentId(IComponent)"/>.</param>
/// <param name="initialParameters">The <see cref="ParameterView"/>with the initial parameters to use for rendering.</param>
/// <param name="initialParameters">The <see cref="ParameterView"/> with the initial or updated parameters to use for rendering.</param>
/// <remarks>
/// Rendering a root component is an asynchronous operation. Clients may choose to not await the returned task to
/// start, but not wait for the entire render to complete.
/// </remarks>
protected async Task RenderRootComponentAsync(int componentId, ParameterView initialParameters)
{
if (Interlocked.CompareExchange(ref _pendingTasks, new List<Task>(), null) != null)
{
throw new InvalidOperationException("There is an ongoing rendering in progress.");
}
Dispatcher.AssertAccess();

// Since this is a "render root" operation being invoked from outside the system, we start tracking
// any async tasks from this point until we reach quiescence. This allows external code such as prerendering
// to know when the renderer has some finished output. We don't track async tasks at other times
// because nobody would be waiting for quiescence at other times.
// Having a nonnull value for _pendingTasks is what signals that we should be capturing the async tasks.
_pendingTasks ??= new();

// During the rendering process we keep a list of components performing work in _pendingTasks.
// _renderer.AddToPendingTasks will be called by ComponentState.SetDirectParameters to add the
// the Task produced by Component.SetParametersAsync to _pendingTasks in order to track the
// remaining work.
// During the synchronous rendering process we don't wait for the pending asynchronous
// work to finish as it will simply trigger new renders that will be handled afterwards.
// During the asynchronous rendering process we want to wait up until all components have
// finished rendering so that we can produce the complete output.
var componentState = GetRequiredComponentState(componentId);
if (TestableMetadataUpdate.IsSupported)
{
// when we're doing hot-reload, stash away the parameters used while rendering root components.
// When we're doing hot-reload, stash away the parameters used while rendering root components.
// We'll use this to trigger re-renders on hot reload updates.
_rootComponents ??= new();
_rootComponents.Add((componentState, initialParameters.Clone()));
_rootComponentsLatestParameters ??= new();
_rootComponentsLatestParameters[componentId] = initialParameters.Clone();
}

componentState.SetDirectParameters(initialParameters);

try
await WaitForQuiescence();
Debug.Assert(_pendingTasks == null);
}

/// <summary>
/// Removes the specified component from the renderer, causing the component and its
/// descendants to be disposed.
/// </summary>
/// <param name="componentId">The ID of the root component.</param>
protected void RemoveRootComponent(int componentId)
{
Dispatcher.AssertAccess();

var rootComponentState = GetRequiredComponentState(componentId);
if (rootComponentState.ParentComponentState is not null)
{
await ProcessAsynchronousWork();
Debug.Assert(_pendingTasks.Count == 0);
throw new InvalidOperationException("The specified component is not a root component");
}
finally

// This assumes there isn't currently a batch in progress, and will throw if there is.
// Currently there's no known scenario where we need to support calling RemoveRootComponentAsync
// during a batch, but if a scenario emerges we can add support.
_batchBuilder.ComponentDisposalQueue.Enqueue(componentId);
if (TestableMetadataUpdate.IsSupported)
{
_pendingTasks = null;
_rootComponentsLatestParameters?.Remove(componentId);
}

ProcessRenderQueue();
Copy link
Member

Choose a reason for hiding this comment

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

Will this trigger the disposal of all the components in the tree? I remember having to do something more "sophisticated" here (triggering an empty render).

Copy link
Member Author

Choose a reason for hiding this comment

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

It does - disposal is recursive. There's a unit test for that too.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I'm glad we found a simpler way, the way I did it was a bit convoluted

}

/// <summary>
Expand All @@ -254,21 +274,43 @@ protected async Task RenderRootComponentAsync(int componentId, ParameterView ini
/// <param name="exception">The <see cref="Exception"/>.</param>
protected abstract void HandleException(Exception exception);

private async Task ProcessAsynchronousWork()
private async Task WaitForQuiescence()
{
// Child components SetParametersAsync are stored in the queue of pending tasks,
// which might trigger further renders.
while (_pendingTasks.Count > 0)
// If there's already a loop waiting for quiescence, just join it
if (_ongoingQuiescenceTask is not null)
{
// Create a Task that represents the remaining ongoing work for the rendering process
var pendingWork = Task.WhenAll(_pendingTasks);
await _ongoingQuiescenceTask;
return;
}

// Clear all pending work.
_pendingTasks.Clear();
try
{
_ongoingQuiescenceTask = ProcessAsynchronousWork();
await _ongoingQuiescenceTask;
}
finally
{
Debug.Assert(_pendingTasks.Count == 0);
_pendingTasks = null;
_ongoingQuiescenceTask = null;
}

async Task ProcessAsynchronousWork()
{
// Child components SetParametersAsync are stored in the queue of pending tasks,
// which might trigger further renders.
while (_pendingTasks.Count > 0)
{
// Create a Task that represents the remaining ongoing work for the rendering process
var pendingWork = Task.WhenAll(_pendingTasks);

// Clear all pending work.
_pendingTasks.Clear();

// new work might be added before we check again as a result of waiting for all
// the child components to finish executing SetParametersAsync
await pendingWork;
// new work might be added before we check again as a result of waiting for all
// the child components to finish executing SetParametersAsync
await pendingWork;
}
}
}

Expand Down Expand Up @@ -544,7 +586,17 @@ private void ProcessRenderQueue()
{
if (_batchBuilder.ComponentRenderQueue.Count == 0)
{
return;
if (_batchBuilder.ComponentDisposalQueue.Count == 0)
{
// Nothing to do
return;
}
else
{
// Normally we process the disposal queue after each component rendering step,
// but in this case disposal is the only pending action so far
ProcessDisposalQueueInExistingBatch();
}
}

// Process render queue until empty
Expand Down Expand Up @@ -714,9 +766,13 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry)
HandleExceptionViaErrorBoundary(renderFragmentException, componentState);
}

List<Exception> exceptions = null;

// Process disposal queue now in case it causes further component renders to be enqueued
ProcessDisposalQueueInExistingBatch();
}

private void ProcessDisposalQueueInExistingBatch()
{
List<Exception> exceptions = null;
while (_batchBuilder.ComponentDisposalQueue.Count > 0)
{
var disposeComponentId = _batchBuilder.ComponentDisposalQueue.Dequeue();
Expand Down
8 changes: 4 additions & 4 deletions src/Components/Components/test/EventCallbackFactoryBinderExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ public async Task CreateBinder_DateTime()
var expectedValue = new DateTime(2018, 3, 4, 1, 2, 3);

// Act
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.InvariantCulture), });
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.CurrentCulture), });

Assert.Equal(expectedValue, value);
Assert.Equal(1, component.Count);
Expand All @@ -415,7 +415,7 @@ public async Task CreateBinder_NullableDateTime()
var expectedValue = new DateTime(2018, 3, 4, 1, 2, 3);

// Act
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.InvariantCulture), });
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.CurrentCulture), });

Assert.Equal(expectedValue, value);
Assert.Equal(1, component.Count);
Expand Down Expand Up @@ -474,7 +474,7 @@ public async Task CreateBinder_DateTimeOffset()
var expectedValue = new DateTime(2018, 3, 4, 1, 2, 3);

// Act
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.InvariantCulture), });
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.CurrentCulture), });

Assert.Equal(expectedValue, value);
Assert.Equal(1, component.Count);
Expand All @@ -493,7 +493,7 @@ public async Task CreateBinder_NullableDateTimeOffset()
var expectedValue = new DateTime(2018, 3, 4, 1, 2, 3);

// Act
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.InvariantCulture), });
await binder.InvokeAsync(new ChangeEventArgs() { Value = expectedValue.ToString(CultureInfo.CurrentCulture), });
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be unrelated, won't CurrentCulture cause issues with machines on other locales?

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The test was wrong before. Binding works with culture-specific values. The parsing was already culture-specific, so it was incorrect to test a culture-invariant input, and was failing on my machine (using months for days, and vice-versa).

The only reason it was passing in CI before was due to the cultural imperialism of CultureInfo.InvariantCulture being equivalent to US date formatting. It should now work on all machines, each of them doing both formatting and parsing with their own local formatting.

Copy link
Member

Choose a reason for hiding this comment

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

the cultural imperialism of CultureInfo.InvariantCulture being equivalent to US date formatting

lol, fair enough :).

I only brought it up because we ran into issues in the past in this area.


Assert.Equal(expectedValue, value);
Assert.Equal(1, component.Count);
Expand Down
Loading