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

Store event handler IDs as longs. Fixes #12058 #12305

Merged
merged 5 commits into from
Jul 18, 2019
Merged

Store event handler IDs as longs. Fixes #12058 #12305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86b5aeb
Change event handler IDs to be longs
SteveSandersonMS Jul 18, 2019
d5ecb78
Update unit tests
SteveSandersonMS Jul 18, 2019
c901793
Enable detailed errors for test app
SteveSandersonMS Jul 18, 2019
2ccf583
CR: Explicitly check if we exceed Number.MAX_SAFE_INTEGER
SteveSandersonMS Jul 18, 2019
709761d
Update ref assemblies
SteveSandersonMS Jul 18, 2019
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
2 changes: 1 addition & 1 deletion src/Components/Blazor/Blazor/ref/Microsoft.AspNetCore.Blazor.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public partial class WebAssemblyRenderer : Microsoft.AspNetCore.Components.Rende
public override Microsoft.AspNetCore.Components.Dispatcher Dispatcher { get { throw null; } }
public System.Threading.Tasks.Task AddComponentAsync(System.Type componentType, string domElementSelector) { throw null; }
public System.Threading.Tasks.Task AddComponentAsync<TComponent>(string domElementSelector) where TComponent : Microsoft.AspNetCore.Components.IComponent { throw null; }
public override System.Threading.Tasks.Task DispatchEventAsync(int eventHandlerId, Microsoft.AspNetCore.Components.Rendering.EventFieldInfo eventFieldInfo, Microsoft.AspNetCore.Components.UIEventArgs eventArgs) { throw null; }
public override System.Threading.Tasks.Task DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.Rendering.EventFieldInfo eventFieldInfo, Microsoft.AspNetCore.Components.UIEventArgs eventArgs) { throw null; }
protected override void Dispose(bool disposing) { }
protected override void HandleException(System.Exception exception) { }
protected override System.Threading.Tasks.Task UpdateDisplayAsync(in Microsoft.AspNetCore.Components.Rendering.RenderBatch batch) { throw null; }
Expand Down
6 changes: 3 additions & 3 deletions src/Components/Blazor/Blazor/src/Rendering/WebAssemblyRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ protected override void HandleException(Exception exception)
}

/// <inheritdoc />
public override Task DispatchEventAsync(int eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)
public override Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)
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 our events are always positive and that we are never going to wrap around an ulong, but I'm worried about using unsigned types in here just because we pretty much don't ever use them and because I'm not confident on all the integration issues this might bring, from serialization, to method invocation, etc. Id rather if possible we simply use a long value as it achieves our goal and is less likely to cause issues in the future.

That said, It might be a case of me being paranoid about this for no reason. I'm just scared of the unknowns using this brings, as I don't think there's an API (or very few APIs) in ASP.NET or .NET that specifically use them.

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 picked ulong because it was easier to make robust:

  • Ints don’t implicitly convert to or from it (unlike long), so you get compile time checking that we’re consistent everywhere in .NET
  • The JS-side code for reading uint64 is simpler and harder to get wrong than if it was reading int64, as there’s no sign bit to account for.

Copy link
Member

Choose a reason for hiding this comment

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

That said, It might be a case of me being paranoid about this for no reason. I'm just scared of the unknowns using this brings, as I don't think there's an API (or very few APIs) in ASP.NET or .NET that specifically use them.

I don't share your concerns 😆 I've used ulong before.

{
// Be sure we only run one event handler at once. Although they couldn't run
// simultaneously anyway (there's only one thread), they could run nested on
Expand Down Expand Up @@ -183,12 +183,12 @@ private async Task ProcessNextDeferredEventAsync()

readonly struct IncomingEventInfo
{
public readonly int EventHandlerId;
public readonly ulong EventHandlerId;
public readonly EventFieldInfo EventFieldInfo;
public readonly UIEventArgs EventArgs;
public readonly TaskCompletionSource<object> TaskCompletionSource;

public IncomingEventInfo(int eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)
public IncomingEventInfo(ulong eventHandlerId, EventFieldInfo eventFieldInfo, UIEventArgs eventArgs)
{
EventHandlerId = eventHandlerId;
EventFieldInfo = eventFieldInfo;
Expand Down
4 changes: 2 additions & 2 deletions src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ public readonly partial struct RenderBatch
{
private readonly object _dummy;
public Microsoft.AspNetCore.Components.RenderTree.ArrayRange<int> DisposedComponentIDs { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Components.RenderTree.ArrayRange<int> DisposedEventHandlerIDs { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Components.RenderTree.ArrayRange<ulong> DisposedEventHandlerIDs { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Components.RenderTree.ArrayRange<Microsoft.AspNetCore.Components.RenderTree.RenderTreeFrame> ReferenceFrames { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public Microsoft.AspNetCore.Components.RenderTree.ArrayRange<Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiff> UpdatedComponents { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
}
Expand All @@ -735,7 +735,7 @@ public Renderer(System.IServiceProvider serviceProvider, Microsoft.Extensions.Lo
public event System.UnhandledExceptionEventHandler UnhandledSynchronizationException { add { } remove { } }
protected internal virtual void AddToRenderQueue(int componentId, Microsoft.AspNetCore.Components.RenderFragment renderFragment) { }
protected internal int AssignRootComponentId(Microsoft.AspNetCore.Components.IComponent component) { throw null; }
public virtual System.Threading.Tasks.Task DispatchEventAsync(int eventHandlerId, Microsoft.AspNetCore.Components.Rendering.EventFieldInfo fieldInfo, Microsoft.AspNetCore.Components.UIEventArgs eventArgs) { throw null; }
public virtual System.Threading.Tasks.Task DispatchEventAsync(ulong eventHandlerId, Microsoft.AspNetCore.Components.Rendering.EventFieldInfo fieldInfo, Microsoft.AspNetCore.Components.UIEventArgs eventArgs) { throw null; }
public void Dispose() { }
protected virtual void Dispose(bool disposing) { }
protected abstract void HandleException(System.Exception exception);
Expand Down
8 changes: 4 additions & 4 deletions src/Components/Components/src/RenderTree/RenderTreeFrame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Components.RenderTree
/// <summary>
/// Represents an entry in a tree of user interface (UI) items.
/// </summary>
[StructLayout(LayoutKind.Explicit)]
[StructLayout(LayoutKind.Explicit, Pack = 4)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Pack = 4 is now needed because by default, Mono picks a struct length that's the smallest multiple of the largest field size. The largest field is now 8 bytes, so it would pick struct length 40. Explicitly specifying Pack=4 lets it pick a multiple of 4, so with this it picks struct length 36.

The existing TS-side code already expects 36. We could change it to 40 but with this line, there's no need.

public readonly struct RenderTreeFrame
{
// Note that the struct layout has to be valid in both 32-bit and 64-bit runtime platforms,
Expand Down Expand Up @@ -90,7 +90,7 @@ public readonly struct RenderTreeFrame
/// If the <see cref="FrameType"/> property equals <see cref="RenderTreeFrameType.Attribute"/>
/// gets the ID of the corresponding event handler, if any.
/// </summary>
[FieldOffset(8)] public readonly int AttributeEventHandlerId;
[FieldOffset(8)] public readonly ulong AttributeEventHandlerId;

/// <summary>
/// If the <see cref="FrameType"/> property equals <see cref="RenderTreeFrameType.Attribute"/>,
Expand Down Expand Up @@ -267,7 +267,7 @@ private RenderTreeFrame(int sequence, bool isMarkup, string textOrMarkup)
}

// Attribute constructor
private RenderTreeFrame(int sequence, string attributeName, object attributeValue, int attributeEventHandlerId, string attributeEventUpdatesAttributeName)
private RenderTreeFrame(int sequence, string attributeName, object attributeValue, ulong attributeEventHandlerId, string attributeEventUpdatesAttributeName)
: this()
{
FrameType = RenderTreeFrameType.Attribute;
Expand Down Expand Up @@ -337,7 +337,7 @@ internal RenderTreeFrame WithAttributeSequence(int sequence)
internal RenderTreeFrame WithComponent(ComponentState componentState)
=> new RenderTreeFrame(Sequence, componentSubtreeLength: ComponentSubtreeLength, ComponentType, componentState, ComponentKey);

internal RenderTreeFrame WithAttributeEventHandlerId(int eventHandlerId)
internal RenderTreeFrame WithAttributeEventHandlerId(ulong eventHandlerId)
=> new RenderTreeFrame(Sequence, attributeName: AttributeName, AttributeValue, eventHandlerId, AttributeEventUpdatesAttributeName);

internal RenderTreeFrame WithAttributeValue(object attributeValue)
Expand Down
6 changes: 3 additions & 3 deletions src/Components/Components/src/Rendering/RenderBatch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.AspNetCore.Components.RenderTree;
Expand Down Expand Up @@ -30,13 +30,13 @@ public readonly struct RenderBatch
/// <summary>
/// Gets the IDs of the event handlers that were disposed.
/// </summary>
public ArrayRange<int> DisposedEventHandlerIDs { get; }
public ArrayRange<ulong> DisposedEventHandlerIDs { get; }

internal RenderBatch(
ArrayRange<RenderTreeDiff> updatedComponents,
ArrayRange<RenderTreeFrame> referenceFrames,
ArrayRange<int> disposedComponentIDs,
ArrayRange<int> disposedEventHandlerIDs)
ArrayRange<ulong> disposedEventHandlerIDs)
{
UpdatedComponents = updatedComponents;
ReferenceFrames = referenceFrames;
Expand Down
2 changes: 1 addition & 1 deletion src/Components/Components/src/Rendering/RenderBatchBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ internal class RenderBatchBuilder : IDisposable
// Primary result data
public ArrayBuilder<RenderTreeDiff> UpdatedComponentDiffs { get; } = new ArrayBuilder<RenderTreeDiff>();
public ArrayBuilder<int> DisposedComponentIds { get; } = new ArrayBuilder<int>();
Copy link
Member

Choose a reason for hiding this comment

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

Should component ids be on the same boat? I can imagine that if we don't want to ever wrap around an event, it might be even more important for components, as it might be the case that 1 event triggers multiple component instantiations.

Thoughts?

I know the struct layout might get more complicated if we add 4 more bytes. But I think its a matter of being better safe than sorry.

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’m open to it, but it will be quite a lot more disruptive than the event handler ID change, as component IDs are used in so many more places.

To run out of IDs, you’d need to do something like instantiate 1000 components per second continuously for 25 days (in one circuit). I think we could say that is impractical for lots of reasons.

One cheaper option would just be to change any public API for component IDs to be ulong, which leaves us the option of changing the underlying storage in the future if we want.

public ArrayBuilder<int> DisposedEventHandlerIds { get; } = new ArrayBuilder<int>();
public ArrayBuilder<ulong> DisposedEventHandlerIds { get; } = new ArrayBuilder<ulong>();

// Buffers referenced by UpdatedComponentDiffs
public ArrayBuilder<RenderTreeEdit> EditsBuffer { get; } = new ArrayBuilder<RenderTreeEdit>(64);
Expand Down
2 changes: 1 addition & 1 deletion src/Components/Components/src/Rendering/RenderTreeUpdater.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace Microsoft.AspNetCore.Components.Rendering
{
internal class RenderTreeUpdater
{
public static void UpdateToMatchClientState(RenderTreeBuilder renderTreeBuilder, int eventHandlerId, object newFieldValue)
public static void UpdateToMatchClientState(RenderTreeBuilder renderTreeBuilder, ulong eventHandlerId, object newFieldValue)
{
// We only allow the client to supply string or bool currently, since those are the only kinds of
// values we output on attributes that go to the client
Expand Down
6 changes: 3 additions & 3 deletions src/Components/Components/src/Rendering/Renderer.Log.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ internal static class Log
private static readonly Action<ILogger, int, Type, Exception> _disposingComponent =
LoggerMessage.Define<int, Type>(LogLevel.Debug, new EventId(4, "DisposingComponent"), "Disposing component {ComponentId} of type {ComponentType}");

private static readonly Action<ILogger, int, string, Exception> _handlingEvent =
LoggerMessage.Define<int, string>(LogLevel.Debug, new EventId(5, "HandlingEvent"), "Handling event {EventId} of type '{EventType}'");
private static readonly Action<ILogger, ulong, string, Exception> _handlingEvent =
LoggerMessage.Define<ulong, string>(LogLevel.Debug, new EventId(5, "HandlingEvent"), "Handling event {EventId} of type '{EventType}'");

public static void InitializingComponent(ILogger logger, ComponentState componentState, ComponentState parentComponentState)
{
Expand Down Expand Up @@ -56,7 +56,7 @@ internal static void DisposingComponent(ILogger<Renderer> logger, ComponentState
}
}

internal static void HandlingEvent(ILogger<Renderer> logger, int eventHandlerId, UIEventArgs eventArgs)
internal static void HandlingEvent(ILogger<Renderer> logger, ulong eventHandlerId, UIEventArgs eventArgs)
{
_handlingEvent(logger, eventHandlerId, eventArgs?.Type ?? "null", null);
}
Expand Down
18 changes: 9 additions & 9 deletions src/Components/Components/src/Rendering/Renderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ public abstract partial class Renderer : IDisposable
private readonly IServiceProvider _serviceProvider;
private readonly Dictionary<int, ComponentState> _componentStateById = new Dictionary<int, ComponentState>();
private readonly RenderBatchBuilder _batchBuilder = new RenderBatchBuilder();
private readonly Dictionary<int, EventCallback> _eventBindings = new Dictionary<int, EventCallback>();
private readonly Dictionary<int, int> _eventHandlerIdReplacements = new Dictionary<int, int>();
private readonly Dictionary<ulong, EventCallback> _eventBindings = new Dictionary<ulong, EventCallback>();
private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>();
private readonly ILogger<Renderer> _logger;

private int _nextComponentId = 0; // TODO: change to 'long' when Mono .NET->JS interop supports it
private bool _isBatchInProgress;
private int _lastEventHandlerId = 0;
private ulong _lastEventHandlerId;
private List<Task> _pendingTasks;

/// <summary>
Expand Down Expand Up @@ -206,7 +206,7 @@ private ComponentState AttachAndInitComponent(IComponent component, int parentCo
/// A <see cref="Task"/> which will complete once all asynchronous processing related to the event
/// has completed.
/// </returns>
public virtual Task DispatchEventAsync(int eventHandlerId, EventFieldInfo fieldInfo, UIEventArgs eventArgs)
public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo fieldInfo, UIEventArgs eventArgs)
{
EnsureSynchronizationContext();

Expand Down Expand Up @@ -354,15 +354,15 @@ protected internal virtual void AddToRenderQueue(int componentId, RenderFragment
}
}

internal void TrackReplacedEventHandlerId(int oldEventHandlerId, int newEventHandlerId)
internal void TrackReplacedEventHandlerId(ulong oldEventHandlerId, ulong newEventHandlerId)
{
// Tracking the chain of old->new replacements allows us to interpret incoming EventFieldInfo
// values even if they refer to an event handler ID that's since been superseded. This is essential
// for tree patching to work in an async environment.
_eventHandlerIdReplacements.Add(oldEventHandlerId, newEventHandlerId);
}

private int FindLatestEventHandlerIdInChain(int eventHandlerId)
private ulong FindLatestEventHandlerIdInChain(ulong eventHandlerId)
{
while (_eventHandlerIdReplacements.TryGetValue(eventHandlerId, out var replacementEventHandlerId))
{
Expand Down Expand Up @@ -573,7 +573,7 @@ private void RenderInExistingBatch(RenderQueueEntry renderQueueEntry)
}
}

private void RemoveEventHandlerIds(ArrayRange<int> eventHandlerIds, Task afterTaskIgnoreErrors)
private void RemoveEventHandlerIds(ArrayRange<ulong> eventHandlerIds, Task afterTaskIgnoreErrors)
{
if (eventHandlerIds.Count == 0)
{
Expand All @@ -598,7 +598,7 @@ private void RemoveEventHandlerIds(ArrayRange<int> eventHandlerIds, Task afterTa

// Factor out the async part into a separate local method purely so, in the
// synchronous case, there's no state machine or task construction
async Task ContinueAfterTask(ArrayRange<int> eventHandlerIds, Task afterTaskIgnoreErrors)
async Task ContinueAfterTask(ArrayRange<ulong> eventHandlerIds, Task afterTaskIgnoreErrors)
{
// We need to delay the actual removal (e.g., until we've confirmed the client
// has processed the batch and hence can be sure not to reuse the handler IDs
Expand Down Expand Up @@ -637,7 +637,7 @@ private async Task GetErrorHandledTask(Task taskToHandle)
}
}

private void UpdateRenderTreeToMatchClientState(int eventHandlerId, EventFieldInfo fieldInfo)
private void UpdateRenderTreeToMatchClientState(ulong eventHandlerId, EventFieldInfo fieldInfo)
{
var componentState = GetOptionalComponentState(fieldInfo.ComponentId);
if (componentState != null)
Expand Down
6 changes: 3 additions & 3 deletions src/Components/Components/test/RenderTreeDiffBuilderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,7 @@ public void RecognizesAttributeEventHandlerValuesChanged()
Assert.Equal(0, entry.ReferenceFrameIndex);
});
AssertFrame.Attribute(referenceFrames[0], "onbar", addedHandler);
Assert.NotEqual(0, removedEventHandlerFrame.AttributeEventHandlerId);
Assert.NotEqual(default, removedEventHandlerFrame.AttributeEventHandlerId);
Assert.Equal(
new[] { removedEventHandlerFrame.AttributeEventHandlerId },
batchBuilder.DisposedEventHandlerIDs.AsEnumerable());
Expand Down Expand Up @@ -1592,7 +1592,7 @@ public void PreservesEventHandlerIdsForRetainedEventHandlers()
Assert.Empty(result.Edits);
AssertFrame.Attribute(oldAttributeFrame, "ontest", retainedHandler);
AssertFrame.Attribute(newAttributeFrame, "ontest", retainedHandler);
Assert.NotEqual(0, oldAttributeFrame.AttributeEventHandlerId);
Assert.NotEqual(default, oldAttributeFrame.AttributeEventHandlerId);
Assert.Equal(oldAttributeFrame.AttributeEventHandlerId, newAttributeFrame.AttributeEventHandlerId);
Assert.Empty(batchBuilder.DisposedEventHandlerIDs.AsEnumerable());
}
Expand All @@ -1619,7 +1619,7 @@ public void PreservesEventHandlerIdsForRetainedEventHandlers_SlowPath()
Assert.Single(result.Edits);
AssertFrame.Attribute(oldAttributeFrame, "ontest", retainedHandler);
AssertFrame.Attribute(newAttributeFrame, "ontest", retainedHandler);
Assert.NotEqual(0, oldAttributeFrame.AttributeEventHandlerId);
Assert.NotEqual(default, oldAttributeFrame.AttributeEventHandlerId);
Assert.Equal(oldAttributeFrame.AttributeEventHandlerId, newAttributeFrame.AttributeEventHandlerId);
Assert.Empty(batchBuilder.DisposedEventHandlerIDs.AsEnumerable());
}
Expand Down
4 changes: 2 additions & 2 deletions src/Components/Components/test/RendererTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3313,7 +3313,7 @@ public void EventFieldInfoCanPatchTreeSoDiffDoesNotUpdateAttribute(string oldVal
Assert.Equal(RenderTreeEditType.SetAttribute, edit.Type);
var attributeFrame = batch2.ReferenceFrames[edit.ReferenceFrameIndex];
AssertFrame.Attribute(attributeFrame, "ontestevent", typeof(Action<UIChangeEventArgs>));
Assert.NotEqual(0, attributeFrame.AttributeEventHandlerId);
Assert.NotEqual(default, attributeFrame.AttributeEventHandlerId);
Assert.NotEqual(eventHandlerId, attributeFrame.AttributeEventHandlerId);
});
}
Expand Down Expand Up @@ -3365,7 +3365,7 @@ public void EventFieldInfoWorksWhenEventHandlerIdWasSuperseded()
Assert.Equal(RenderTreeEditType.SetAttribute, edit.Type);
var attributeFrame = latestBatch.ReferenceFrames[edit.ReferenceFrameIndex];
AssertFrame.Attribute(attributeFrame, "ontestevent", typeof(Action<UIChangeEventArgs>));
Assert.NotEqual(0, attributeFrame.AttributeEventHandlerId);
Assert.NotEqual(default, attributeFrame.AttributeEventHandlerId);
Assert.NotEqual(eventHandlerId, attributeFrame.AttributeEventHandlerId);
});
}
Expand Down
Loading