Skip to content

Dispatch dispose to sync context. Fixes #32411 #35217

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 3 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 19 additions & 6 deletions src/Components/Components/src/RenderTree/Renderer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable
private ulong _lastEventHandlerId;
private List<Task>? _pendingTasks;
private Task? _disposeTask;
private bool _rendererIsDisposed;

/// <summary>
/// Allows the caller to handle exceptions from the SynchronizationContext when one is available.
Expand Down Expand Up @@ -128,7 +129,7 @@ private static IComponentActivator GetComponentActivatorOrDefault(IServiceProvid
/// <summary>
/// Gets whether the renderer has been disposed.
/// </summary>
internal bool Disposed { get; private set; }
internal bool Disposed => _rendererIsDisposed;

private async void RenderRootComponentsOnHotReload()
{
Expand Down Expand Up @@ -582,9 +583,10 @@ private ComponentState GetRequiredRootComponentState(int componentId)
/// </summary>
protected virtual void ProcessPendingRender()
{
if (Disposed)
if (_rendererIsDisposed)
{
throw new ObjectDisposedException(nameof(Renderer), "Cannot process pending renders after the renderer has been disposed.");
// Once we're disposed, we'll disregard further attempts to render anything
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously we didn't get this far, because an earlier Dispatcher.CheckAccess call would have thrown. But now we do get this far so we need to not regard it as an error any more. It's now just a no-op to try rendering after disposal.

}

ProcessRenderQueue();
Expand Down Expand Up @@ -974,7 +976,17 @@ private void HandleExceptionViaErrorBoundary(Exception error, ComponentState? er
/// <param name="disposing"><see langword="true"/> if this method is being invoked by <see cref="IDisposable.Dispose"/>, otherwise <see langword="false"/>.</param>
protected virtual void Dispose(bool disposing)
{
Disposed = true;
if (!Dispatcher.CheckAccess())
{
// It's important that we only call the components' Dispose/DisposeAsync lifecycle methods
// on the sync context, like other lifecycle methods. In almost all cases we'd already be
// on the sync context here since DisposeAsync dispatches, but just in case someone is using
// Dispose directly, we'll dispatch and block.
Dispatcher.InvokeAsync(() => Dispose(disposing)).Wait();
return;
}

_rendererIsDisposed = true;

if (TestableMetadataUpdate.IsSupported)
{
Expand Down Expand Up @@ -1079,7 +1091,7 @@ public void Dispose()
/// <inheritdoc />
Copy link
Member

Choose a reason for hiding this comment

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

I know it's not the topic of this PR, however since we are already updating this, I would like to take the chance to suggest that we mark Dispose here as obsolete and remove it in the future. The renderer is an "internal" API and for all intents and purposes we are the only ones (with a few exceptions) using it.

The renderer implementing IDisposable is a remnant of olden times when IAsyncDisposable didn't exist, and our programming model is highly skewed towards async computing, so we should take the opportunity now to start obsoleting these and avoid having to worry about them in the future.

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'd be fine with us doing this, but don't want to make it part of this PR as there's relatively little time for me to do the remaining things that need to be done this week. Do you want to file an issue? Maybe we can do it in RC2.

public async ValueTask DisposeAsync()
{
if (Disposed)
if (_rendererIsDisposed)
{
return;
}
Expand All @@ -1090,7 +1102,8 @@ public async ValueTask DisposeAsync()
}
else
{
Dispose();
await Dispatcher.InvokeAsync(Dispose);

if (_disposeTask != null)
{
await _disposeTask;
Expand Down
62 changes: 58 additions & 4 deletions src/Components/Components/test/RendererTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3930,15 +3930,24 @@ public void DisposingRenderer_DisposesTopLevelComponents()
}

[Fact]
public void DisposingRenderer_RejectsAttemptsToStartMoreRenderBatches()
public void DisposingRenderer_DisregardsAttemptsToStartMoreRenderBatches()
{
// Arrange
var renderer = new TestRenderer();
var component = new TestComponent(builder =>
{
builder.OpenElement(0, "my element");
builder.AddContent(1, "some text");
builder.CloseElement();
});

// Act
renderer.AssignRootComponentId(component);
renderer.Dispose();
component.TriggerRender();

// Act/Assert
var ex = Assert.Throws<ObjectDisposedException>(() => renderer.ProcessPendingRender());
Assert.Contains("Cannot process pending renders after the renderer has been disposed.", ex.Message);
// Assert
Assert.Empty(renderer.Batches);
}

[Fact]
Expand Down Expand Up @@ -4679,6 +4688,51 @@ public void RemoveRootComponentHandlesDisposalExceptions()
Assert.Same(exception2, renderer.HandledExceptions[1]);
}

[Fact]
public void DisposeCallsComponentDisposeOnSyncContext()
{
// Arrange
var renderer = new TestRenderer();
var wasOnSyncContext = false;
var component = new DisposableComponent
{
DisposeAction = () =>
{
wasOnSyncContext = renderer.Dispatcher.CheckAccess();
}
};

// Act
var componentId = renderer.AssignRootComponentId(component);
renderer.Dispose();

// Assert
Assert.True(wasOnSyncContext);
}

[Fact]
public async Task DisposeAsyncCallsComponentDisposeAsyncOnSyncContext()
{
// Arrange
var renderer = new TestRenderer();
var wasOnSyncContext = false;
var component = new AsyncDisposableComponent
{
AsyncDisposeAction = () =>
{
wasOnSyncContext = renderer.Dispatcher.CheckAccess();
return ValueTask.CompletedTask;
}
};

// Act
var componentId = renderer.AssignRootComponentId(component);
await renderer.DisposeAsync();

// Assert
Assert.True(wasOnSyncContext);
}

private class TestComponentActivator<TResult> : IComponentActivator where TResult : IComponent, new()
{
public List<Type> RequestedComponentTypes { get; } = new List<Type>();
Expand Down