From a46be4e200b11b87876cc7cc9f37939a67ca40dc Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Sun, 5 Mar 2017 02:11:13 +0100 Subject: [PATCH] Ensure correct thread for AvaloniaProperty access. Previously we ensured that `AvaloniaObject.SetValue` was run on the main thread. This makes sure that `GetValue`, `IsSet` and `ClearValue` are also run on the main thread. Unit testing this turned out to be more complicated than expected, because `Dispatcher` keeps a hold of a reference to the first `IPlatformThreadingInterface` it sees, so made `UnitTestApplication` able to notify `Dispatcher` that it should update its services. --- Avalonia.v3.ncrunchsolution | 2 +- src/Avalonia.Base/AvaloniaObject.cs | 6 +- src/Avalonia.Base/IPriorityValueOwner.cs | 5 + src/Avalonia.Base/PriorityBindingEntry.cs | 2 + src/Avalonia.Base/PriorityLevel.cs | 18 +- src/Avalonia.Base/PriorityValue.cs | 18 +- src/Avalonia.Base/Properties/AssemblyInfo.cs | 1 + src/Avalonia.Base/Threading/Dispatcher.cs | 40 +++- src/Avalonia.Base/Threading/JobRunner.cs | 13 +- .../Avalonia.Base.UnitTests.csproj | 21 ++ .../AvaloniaObjectTests_Binding.cs | 12 +- .../AvaloniaObjectTests_Threading.cs | 202 ++++++++++++++++++ tests/Avalonia.UnitTests/TestServices.cs | 6 + .../Avalonia.UnitTests/UnitTestApplication.cs | 11 +- 14 files changed, 325 insertions(+), 32 deletions(-) create mode 100644 tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Threading.cs diff --git a/Avalonia.v3.ncrunchsolution b/Avalonia.v3.ncrunchsolution index c2c454eae1b..1b5b0c8930e 100644 --- a/Avalonia.v3.ncrunchsolution +++ b/Avalonia.v3.ncrunchsolution @@ -3,7 +3,7 @@ tests\TestFiles\**.* - False + True .ncrunch True diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 409abfe8fa5..c258070505e 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -181,6 +181,7 @@ select new public void ClearValue(AvaloniaProperty property) { Contract.Requires(property != null); + VerifyAccess(); SetValue(property, AvaloniaProperty.UnsetValue); } @@ -193,6 +194,7 @@ public void ClearValue(AvaloniaProperty property) public object GetValue(AvaloniaProperty property) { Contract.Requires(property != null); + VerifyAccess(); if (property.IsDirect) { @@ -234,7 +236,8 @@ public T GetValue(AvaloniaProperty property) public bool IsSet(AvaloniaProperty property) { Contract.Requires(property != null); - + VerifyAccess(); + PriorityValue value; if (_values.TryGetValue(property, out value)) @@ -332,6 +335,7 @@ public bool IsSet(AvaloniaProperty property) } subscription = source + .Do(_ => VerifyAccess()) .Select(x => CastOrDefault(x, property.PropertyType)) .Do(_ => { }, () => _directBindings.Remove(subscription)) .Subscribe(x => SetDirectValue(property, x)); diff --git a/src/Avalonia.Base/IPriorityValueOwner.cs b/src/Avalonia.Base/IPriorityValueOwner.cs index 57f98c0717d..aeec7209204 100644 --- a/src/Avalonia.Base/IPriorityValueOwner.cs +++ b/src/Avalonia.Base/IPriorityValueOwner.cs @@ -25,5 +25,10 @@ internal interface IPriorityValueOwner /// The source of the change. /// The notification. void BindingNotificationReceived(PriorityValue sender, BindingNotification notification); + + /// + /// Ensures that the current thread is the UI thread. + /// + void VerifyAccess(); } } diff --git a/src/Avalonia.Base/PriorityBindingEntry.cs b/src/Avalonia.Base/PriorityBindingEntry.cs index 25b7bede7e0..eb7cf5414c8 100644 --- a/src/Avalonia.Base/PriorityBindingEntry.cs +++ b/src/Avalonia.Base/PriorityBindingEntry.cs @@ -92,6 +92,8 @@ public void Dispose() private void ValueChanged(object value) { + _owner.Owner.Owner?.VerifyAccess(); + var notification = value as BindingNotification; if (notification != null) diff --git a/src/Avalonia.Base/PriorityLevel.cs b/src/Avalonia.Base/PriorityLevel.cs index 122a6df8217..b25247deaf2 100644 --- a/src/Avalonia.Base/PriorityLevel.cs +++ b/src/Avalonia.Base/PriorityLevel.cs @@ -33,7 +33,6 @@ namespace Avalonia /// internal class PriorityLevel { - private PriorityValue _owner; private object _directValue; private int _nextIndex; @@ -48,13 +47,18 @@ internal class PriorityLevel { Contract.Requires(owner != null); - _owner = owner; + Owner = owner; Priority = priority; Value = _directValue = AvaloniaProperty.UnsetValue; ActiveBindingIndex = -1; Bindings = new LinkedList(); } + /// + /// Gets the owner of the level. + /// + public PriorityValue Owner { get; } + /// /// Gets the priority of this level. /// @@ -73,7 +77,7 @@ public object DirectValue set { Value = _directValue = value; - _owner.LevelValueChanged(this); + Owner.LevelValueChanged(this); } } @@ -131,7 +135,7 @@ public void Changed(PriorityBindingEntry entry) { Value = entry.Value; ActiveBindingIndex = entry.Index; - _owner.LevelValueChanged(this); + Owner.LevelValueChanged(this); } else { @@ -161,7 +165,7 @@ public void Completed(PriorityBindingEntry entry) /// The error. public void Error(PriorityBindingEntry entry, BindingNotification error) { - _owner.LevelError(this, error); + Owner.LevelError(this, error); } /// @@ -175,14 +179,14 @@ private void ActivateFirstBinding() { Value = binding.Value; ActiveBindingIndex = binding.Index; - _owner.LevelValueChanged(this); + Owner.LevelValueChanged(this); return; } } Value = DirectValue; ActiveBindingIndex = -1; - _owner.LevelValueChanged(this); + Owner.LevelValueChanged(this); } } } diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 3f4b405de94..57e28540147 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -26,7 +26,6 @@ namespace Avalonia /// internal class PriorityValue { - private readonly IPriorityValueOwner _owner; private readonly Type _valueType; private readonly SingleOrDictionary _levels = new SingleOrDictionary(); private object _value; @@ -45,7 +44,7 @@ internal class PriorityValue Type valueType, Func validate = null) { - _owner = owner; + Owner = owner; Property = property; _valueType = valueType; _value = AvaloniaProperty.UnsetValue; @@ -53,6 +52,11 @@ internal class PriorityValue _validate = validate; } + /// + /// Gets the owner of the value. + /// + public IPriorityValueOwner Owner { get; } + /// /// Gets the property that the value represents. /// @@ -188,9 +192,9 @@ public void LevelError(PriorityLevel level, BindingNotification error) Logger.Log( LogEventLevel.Error, LogArea.Binding, - _owner, + Owner, "Error in binding to {Target}.{Property}: {Message}", - _owner, + Owner, Property, error.Error.Message); } @@ -264,19 +268,19 @@ private void UpdateValue(object value, int priority) if (notification == null || notification.HasValue) { - _owner?.Changed(this, old, _value); + Owner?.Changed(this, old, _value); } if (notification != null) { - _owner?.BindingNotificationReceived(this, notification); + Owner?.BindingNotificationReceived(this, notification); } } else { Logger.Error( LogArea.Binding, - _owner, + Owner, "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", Property.Name, _valueType, diff --git a/src/Avalonia.Base/Properties/AssemblyInfo.cs b/src/Avalonia.Base/Properties/AssemblyInfo.cs index f57afcdd70b..2ee7378413f 100644 --- a/src/Avalonia.Base/Properties/AssemblyInfo.cs +++ b/src/Avalonia.Base/Properties/AssemblyInfo.cs @@ -6,4 +6,5 @@ [assembly: AssemblyTitle("Avalonia.Base")] [assembly: InternalsVisibleTo("Avalonia.Base.UnitTests")] +[assembly: InternalsVisibleTo("Avalonia.UnitTests")] [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")] \ No newline at end of file diff --git a/src/Avalonia.Base/Threading/Dispatcher.cs b/src/Avalonia.Base/Threading/Dispatcher.cs index aee6a02e732..a8ce1b81157 100644 --- a/src/Avalonia.Base/Threading/Dispatcher.cs +++ b/src/Avalonia.Base/Threading/Dispatcher.cs @@ -17,8 +17,8 @@ namespace Avalonia.Threading /// public class Dispatcher { - private readonly IPlatformThreadingInterface _platform; private readonly JobRunner _jobRunner; + private IPlatformThreadingInterface _platform; public static Dispatcher UIThread { get; } = new Dispatcher(AvaloniaLocator.Current.GetService()); @@ -26,22 +26,31 @@ public class Dispatcher public Dispatcher(IPlatformThreadingInterface platform) { _platform = platform; - if(_platform == null) - //TODO: Unit test mode, fix that somehow - return; _jobRunner = new JobRunner(platform); - _platform.Signaled += _jobRunner.RunJobs; + + if (_platform != null) + { + _platform.Signaled += _jobRunner.RunJobs; + } } + /// + /// Checks that the current thread is the UI thread. + /// public bool CheckAccess() => _platform?.CurrentThreadIsLoopThread ?? true; + /// + /// Checks that the current thread is the UI thread and throws if not. + /// + /// + /// The current thread is not the UI thread. + /// public void VerifyAccess() { if (!CheckAccess()) throw new InvalidOperationException("Call from invalid thread"); } - /// /// Runs the dispatcher's main loop. /// @@ -83,5 +92,24 @@ public void InvokeAsync(Action action, DispatcherPriority priority = DispatcherP { _jobRunner?.Post(action, priority); } + + /// + /// Allows unit tests to change the platform threading interface. + /// + internal void UpdateServices() + { + if (_platform != null) + { + _platform.Signaled -= _jobRunner.RunJobs; + } + + _platform = AvaloniaLocator.Current.GetService(); + _jobRunner.UpdateServices(); + + if (_platform != null) + { + _platform.Signaled += _jobRunner.RunJobs; + } + } } } \ No newline at end of file diff --git a/src/Avalonia.Base/Threading/JobRunner.cs b/src/Avalonia.Base/Threading/JobRunner.cs index 655ef34288e..e212cd9a2e6 100644 --- a/src/Avalonia.Base/Threading/JobRunner.cs +++ b/src/Avalonia.Base/Threading/JobRunner.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Threading; using System.Threading.Tasks; using Avalonia.Platform; @@ -14,8 +13,8 @@ namespace Avalonia.Threading /// internal class JobRunner { - private readonly IPlatformThreadingInterface _platform; private readonly Queue _queue = new Queue(); + private IPlatformThreadingInterface _platform; public JobRunner(IPlatformThreadingInterface platform) { @@ -82,6 +81,14 @@ internal void Post(Action action, DispatcherPriority priority) AddJob(new Job(action, priority, true)); } + /// + /// Allows unit tests to change the platform threading interface. + /// + internal void UpdateServices() + { + _platform = AvaloniaLocator.Current.GetService(); + } + private void AddJob(Job job) { var needWake = false; @@ -91,7 +98,7 @@ private void AddJob(Job job) _queue.Enqueue(job); } if (needWake) - _platform.Signal(); + _platform?.Signal(); } /// diff --git a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj index 07ed7f14ca2..d4cf22ed63e 100644 --- a/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj +++ b/tests/Avalonia.Base.UnitTests/Avalonia.Base.UnitTests.csproj @@ -90,6 +90,7 @@ + @@ -124,6 +125,26 @@ {B09B78D8-9B26-48B0-9149-D64A2F120F3F} Avalonia.Base + + {d2221c82-4a25-4583-9b43-d791e3f6820c} + Avalonia.Controls + + + {62024b2d-53eb-4638-b26b-85eeaa54866e} + Avalonia.Input + + + {42472427-4774-4c81-8aff-9f27b8e31721} + Avalonia.Layout + + + {f1baa01a-f176-4c6a-b39d-5b40bb1b148f} + Avalonia.Styling + + + {eb582467-6abb-43a1-b052-e981ba910e3a} + Avalonia.Visuals + {88060192-33d5-4932-b0f9-8bd2763e857d} Avalonia.UnitTests diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index 5e286305d28..cd8184b1408 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -365,7 +365,7 @@ public void Bind_Logs_Binding_Error() } [Fact] - public async void Bind_With_Scheduler_Executes_On_Scheduler() + public async Task Bind_With_Scheduler_Executes_On_Scheduler() { var target = new Class1(); var source = new Subject(); @@ -375,16 +375,16 @@ public async void Bind_With_Scheduler_Executes_On_Scheduler() threadingInterfaceMock.SetupGet(mock => mock.CurrentThreadIsLoopThread) .Returns(() => Thread.CurrentThread.ManagedThreadId == currentThreadId); - using (AvaloniaLocator.EnterScope()) - { - AvaloniaLocator.CurrentMutable.Bind().ToConstant(threadingInterfaceMock.Object); - AvaloniaLocator.CurrentMutable.Bind().ToConstant(AvaloniaScheduler.Instance); + var services = new TestServices( + scheduler: AvaloniaScheduler.Instance, + threadingInterface: threadingInterfaceMock.Object); + using (UnitTestApplication.Start(services)) + { target.Bind(Class1.QuxProperty, source); await Task.Run(() => source.OnNext(6.7)); } - } /// diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Threading.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Threading.cs new file mode 100644 index 00000000000..588d1ef942d --- /dev/null +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Threading.cs @@ -0,0 +1,202 @@ +// Copyright (c) The Avalonia Project. All rights reserved. +// Licensed under the MIT license. See licence.md file in the project root for full license information. + +using System; +using System.Reactive.Subjects; +using System.Threading; +using System.Threading.Tasks; +using Avalonia.Platform; +using Avalonia.UnitTests; +using Xunit; + +namespace Avalonia.Base.UnitTests +{ + public class AvaloniaObjectTests_Threading + { + [Fact] + public void StyledProperty_GetValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.GetValue(Class1.StyledProperty)); + } + } + + [Fact] + public void StyledProperty_SetValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.SetValue(Class1.StyledProperty, "foo")); + } + } + + [Fact] + public void Setting_StyledProperty_Binding_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => + target.Bind( + Class1.StyledProperty, + new BehaviorSubject("foo"))); + } + } + + [Fact] + public void StyledProperty_Binding_Producing_Value_Should_Throw() + { + var ti = new ThreadingInterface(true); + using (UnitTestApplication.Start(new TestServices(threadingInterface: ti))) + { + var target = new Class1(); + + var source = new BehaviorSubject("foo"); + + target.Bind(Class1.StyledProperty, source); + + ti.CurrentThreadIsLoopThread = false; + Assert.Throws(() => source.OnNext("bar")); + } + } + + [Fact] + public void StyledProperty_ClearValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.ClearValue(Class1.StyledProperty)); + } + } + + [Fact] + public void StyledProperty_IsSet_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.IsSet(Class1.StyledProperty)); + } + } + + [Fact] + public void DirectProperty_GetValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.GetValue(Class1.DirectProperty)); + } + } + + [Fact] + public void DirectProperty_SetValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.SetValue(Class1.DirectProperty, "foo")); + } + } + + [Fact] + public void Setting_DirectProperty_Binding_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => + target.Bind( + Class1.DirectProperty, + new BehaviorSubject("foo"))); + } + } + + [Fact] + public void DirectProperty_Binding_Producing_Value_Should_Throw() + { + var ti = new ThreadingInterface(true); + using (UnitTestApplication.Start(new TestServices(threadingInterface: ti))) + { + var target = new Class1(); + + var source = new BehaviorSubject("foo"); + + target.Bind(Class1.DirectProperty, source); + + ti.CurrentThreadIsLoopThread = false; + Assert.Throws(() => source.OnNext("bar")); + } + } + + [Fact] + public void DirectProperty_ClearValue_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.ClearValue(Class1.DirectProperty)); + } + } + + [Fact] + public void DirectProperty_IsSet_Should_Throw() + { + using (UnitTestApplication.Start(new TestServices(threadingInterface: new ThreadingInterface()))) + { + var target = new Class1(); + + Assert.Throws(() => target.IsSet(Class1.DirectProperty)); + } + } + + private class Class1 : AvaloniaObject + { + public static readonly StyledProperty StyledProperty = + AvaloniaProperty.Register("Foo", "foodefault"); + + public static readonly DirectProperty DirectProperty = + AvaloniaProperty.RegisterDirect("Qux", _ => null, (o, v) => { }); + } + + private class ThreadingInterface : IPlatformThreadingInterface + { + public ThreadingInterface(bool isLoopThread = false) + { + CurrentThreadIsLoopThread = isLoopThread; + } + + public bool CurrentThreadIsLoopThread { get; set; } + + public event Action Signaled; + + public void RunLoop(CancellationToken cancellationToken) + { + throw new NotImplementedException(); + } + + public void Signal() + { + throw new NotImplementedException(); + } + + public IDisposable StartTimer(TimeSpan interval, Action tick) + { + throw new NotImplementedException(); + } + } + } +} diff --git a/tests/Avalonia.UnitTests/TestServices.cs b/tests/Avalonia.UnitTests/TestServices.cs index 8dc838163f4..c98a547295f 100644 --- a/tests/Avalonia.UnitTests/TestServices.cs +++ b/tests/Avalonia.UnitTests/TestServices.cs @@ -12,6 +12,7 @@ using Avalonia.Styling; using Avalonia.Themes.Default; using Avalonia.Rendering; +using System.Reactive.Concurrency; namespace Avalonia.UnitTests { @@ -63,6 +64,7 @@ public class TestServices IRenderer renderer = null, IPlatformRenderInterface renderInterface = null, IRenderLoop renderLoop = null, + IScheduler scheduler = null, IStandardCursorFactory standardCursorFactory = null, IStyler styler = null, Func theme = null, @@ -79,6 +81,7 @@ public class TestServices Renderer = renderer; RenderInterface = renderInterface; RenderLoop = renderLoop; + Scheduler = scheduler; StandardCursorFactory = standardCursorFactory; Styler = styler; Theme = theme; @@ -96,6 +99,7 @@ public class TestServices public IRenderer Renderer { get; } public IPlatformRenderInterface RenderInterface { get; } public IRenderLoop RenderLoop { get; } + public IScheduler Scheduler { get; } public IStandardCursorFactory StandardCursorFactory { get; } public IStyler Styler { get; } public Func Theme { get; } @@ -113,6 +117,7 @@ public class TestServices IRenderer renderer = null, IPlatformRenderInterface renderInterface = null, IRenderLoop renderLoop = null, + IScheduler scheduler = null, IStandardCursorFactory standardCursorFactory = null, IStyler styler = null, Func theme = null, @@ -130,6 +135,7 @@ public class TestServices renderer: renderer ?? Renderer, renderInterface: renderInterface ?? RenderInterface, renderLoop: renderLoop ?? RenderLoop, + scheduler: scheduler ?? Scheduler, standardCursorFactory: standardCursorFactory ?? StandardCursorFactory, styler: styler ?? Styler, theme: theme ?? Theme, diff --git a/tests/Avalonia.UnitTests/UnitTestApplication.cs b/tests/Avalonia.UnitTests/UnitTestApplication.cs index 0dec2184323..a9a1739ee6b 100644 --- a/tests/Avalonia.UnitTests/UnitTestApplication.cs +++ b/tests/Avalonia.UnitTests/UnitTestApplication.cs @@ -8,6 +8,9 @@ using Avalonia.Styling; using Avalonia.Controls; using Avalonia.Rendering; +using Avalonia.Threading; +using System.Reactive.Disposables; +using System.Reactive.Concurrency; namespace Avalonia.UnitTests { @@ -30,7 +33,12 @@ public static IDisposable Start(TestServices services = null) var scope = AvaloniaLocator.EnterScope(); var app = new UnitTestApplication(services); AvaloniaLocator.CurrentMutable.BindToSelf(app); - return scope; + Dispatcher.UIThread.UpdateServices(); + return Disposable.Create(() => + { + scope.Dispose(); + Dispatcher.UIThread.UpdateServices(); + }); } public override void RegisterServices() @@ -47,6 +55,7 @@ public override void RegisterServices() .Bind().ToConstant(Services.RenderInterface) .Bind().ToConstant(Services.RenderLoop) .Bind().ToConstant(Services.ThreadingInterface) + .Bind().ToConstant(Services.Scheduler) .Bind().ToConstant(Services.StandardCursorFactory) .Bind().ToConstant(Services.Styler) .Bind().ToConstant(Services.WindowingPlatform)